2

I'm trying to compile the following C program using GCC and I'm getting an error on line seven because the type taus88_t* is somehow not getting returned properly from the initializing function call called make_taus88(seed);?

error: incompatible types when initializing type 'struct taus88_t *' using type 'taus88_t'|

I've tried using taus88_t TAUS88 = make_taus88(6346456); but that gives more errors/warnings.

taus88main.c||In function 'main':|
taus88main.c|8|error: incompatible type for argument 1 of 'taus88u32'|
taus88.h|21|note: expected 'struct taus88_t *' but argument is of type 'taus88_t'|
taus88main.c|9|error: incompatible type for argument 1 of 'taus88f32'|
taus88.h|22|note: expected 'struct taus88_t *' but argument is of type 'taus88_t'|
taus_88_cpp\taus88main.c|9|warning: unused variable 'numberf32'|
taus_88_cpp\taus88main.c|8|warning: unused variable 'numberu32'|

The project is in 3 files below.

taus88main.c

#include "taus88.h"
#include <stdint.h>

int main()
{

    taus88_t* TAUS88 = make_taus88(6346456);
    u32 numberu32 = taus88u32(TAUS88);
    f32 numberf32 = taus88f32(TAUS88);

    return 0;
}

taus88.h

#ifndef _COMMON_TAUS88_H
#define _COMMON_TAUS88_H

#include <stdint.h>
typedef int8_t    i8;
typedef int16_t   i16;
typedef int32_t   i32;
typedef int64_t   i64;

typedef uint8_t   u8;
typedef uint16_t  u16;
typedef uint32_t  u32;
typedef uint64_t  u64;
typedef float  f32;
typedef double f64;

typedef struct {u32 s1, s2, s3;} taus88_t;

taus88_t make_taus88(u32 seed);
u32 taus88u32(taus88_t *t);
f32 taus88f32(taus88_t *t);

#endif

taus88.c

#include <stdint.h>
#include "taus88.h"

taus88_t make_taus88(u32 seed)
{
  taus88_t t;
  t.s1 = 1243598713U ^ seed; if (t.s1 <  2) t.s1 = 1243598713U;
  t.s2 = 3093459404U ^ seed; if (t.s2 <  8) t.s2 = 3093459404U;
  t.s3 = 1821928721U ^ seed; if (t.s3 < 16) t.s3 = 1821928721U;
  return t;
}

u32 taus88u32(taus88_t *t)
{
  t->s1 = ((t->s1 &  -2) << 12) ^ (((t->s1 << 13) ^  t->s1) >> 19);
  t->s2 = ((t->s2 &  -8) <<  4) ^ (((t->s2 <<  2) ^  t->s2) >> 25);
  t->s3 = ((t->s3 & -16) << 17) ^ (((t->s3 <<  3) ^  t->s3) >> 11);
  return t->s1 ^ t->s2 ^ t->s3;
}

f32 taus88f32(taus88_t *t)
{
  union {u32 i ; f32 f ;} u;
  u.i = 0x3F800000 | (taus88u32(t) >> 9);
  return u.f - 1.0;
}
pandoragami
  • 5,387
  • 15
  • 68
  • 116
  • Unrelated to the main problem, the `taus88f32` function looks like it violates strict aliases rules, http://t.co/NH2GoPu6Sk. – Shafik Yaghmour Jul 27 '13 at 03:00
  • @ShafikYaghmour would that mean catastrophic failure later in life or just semantics? – pandoragami Jul 27 '13 at 03:15
  • The standard does not say that it's not valid, it says that it's undefined behavior ... these are quite different. Things that aren't valid require diagnostics; undefined behavior means that the standard doesn't define what it does ... but an implementation might. – Jim Balter Jul 27 '13 at 04:04
  • "would that mean catastrophic failure later in life or just semantics?" -- it doesn't mean either. It means that the code isn't portable to every possible conforming C implementation, but that's obviously true because it makes assumptions about the internal representation of float values. If you only run it on the systems it is designed for, you're ok ... but knowing which ones those are could be a problem. – Jim Balter Jul 27 '13 at 04:08
  • I have seen many argue that compilers support type punning like this just fine even though it is not standard but to know you need to go into the assembly and see what the compiler actually does. The article I linked in my answer goes into some of those details but at the end of the day the standard says it is `undefined behavior`. – Shafik Yaghmour Jul 27 '13 at 04:09
  • The C standard is only "the end of the day" for language lawyers, not for professional programmers. – Jim Balter Jul 28 '13 at 21:20

3 Answers3

4

So the main problem here is that you are returning a taus8_t and trying to assign that to a taus88_t * which is not valid, if you need to use pointers for reasons that are not obvious from the code then the fix is as follows:

taus88_t* TAUS88 = malloc(sizeof(taus88_t)) ;

*TAUS88 = make_taus88(6346456);

You must remember to call free on the pointer though when you are done. A simpler approach, would be to skip using a pointer and do as follows:

taus88_t TAUS88 ;

TAUS88 = make_taus88(6346456);
u32 numberu32 = taus88u32(&TAUS88);
f32 numberf32 = taus88f32(&TAUS88);

Now you don't have to worry about calling free anymore.

The other issue that I pointed out is that most likely taus88f32 violates strict aliasing rules.

Shafik Yaghmour
  • 154,301
  • 39
  • 440
  • 740
0

What about breaking taus88_t* TAUS88 = make_taus88(6346456); into two lines like

taus88_t* TAUS88;
*TAUS88 = make_taus88(6346456);

http://ideone.com/KVbf79

Snow Blind
  • 1,164
  • 7
  • 12
  • Well `taus88_t* TAUS88 = make_taus88(6346456);` is `pointer=type` but `*TAUS88 = make_taus88(6346456);` is `type=type` right? So it should work. What do you think? – Snow Blind Jul 27 '13 at 03:05
  • I suggest you delete your post before you get downvoted, but thx for trying Snow Blind. – pandoragami Jul 27 '13 at 03:08
  • @SnowBlind Shafiks answer above compiles too but it crashes on the line given. – pandoragami Jul 27 '13 at 03:27
  • @user2555139 Dude first look at the title of your question, then downvote if an answer does not solve it. Runtime error has nothing to do with your original question which two of us answered already. – Snow Blind Jul 27 '13 at 03:31
  • @SnowBlind You need to add a `malloc` for dynamic memory allocation. – Shafik Yaghmour Jul 27 '13 at 03:32
  • I just upvoted you. I don't think I can downvote, takes >50 experience – pandoragami Jul 27 '13 at 03:35
  • Sigh. Just do `taus88_t TAUS88 = make_taus88(6346456);` -- there's no need for malloc. Then use &TAUS88 instead of TAUS88 in the subsequent calls. – Jim Balter Jul 27 '13 at 03:56
0

It looks like make_taus88 returns a taus88_t while you're trying to assign the result to a taus88_t * (pointer to taus88_t)

So, the first big problem is the make_taus88 is defining t locally, then trying to return it. You can't do that, because when make_taus88 terminates, t will be out of scope.

The second is that you're returning a taus88_t and not a taus88_t *.

You can fix both by declaring t as taus88_t * in make_taus88, and initializing it dynamically using something like malloc. Then, you can return t and it will be the correct type, and allocated on the heap, so that when make_taus88 goes out of scope, the memory will still be available.

Note that in make_taus88 you'd also have to change all references of the form t.s1 to t->s1.

Eric
  • 843
  • 6
  • 15
  • Nice feel free to make the changes in the code for me, I sort of understand what you mean but I'm no C fanatic and using Malloc is not my strongest suit. – pandoragami Jul 27 '13 at 03:13
  • You are not correct about returning a struct: http://stackoverflow.com/questions/9653072/return-a-struct-from-a-function-in-c – Shafik Yaghmour Jul 27 '13 at 03:14
  • @ShafikYaghmour: You can return a struct if you want, but you can't assign it to a pointer to the struct. And you still can't return an object that is stored on the stack. – Eric Jul 27 '13 at 03:21
  • 1
    " You can't do that, because when make_taus88 terminates, t will be out of scope." Utter nonsense ... `t` is *copied* so its scope is irrrelevant. – Jim Balter Jul 27 '13 at 03:38
  • 1
    "you can't assign it to a pointer to the struct" -- `*TAUS88` is not a pointer. "And you still can't return an object that is stored on the stack." -- You're simply wrong. You don't understand the language or its memory model. What you cannot return is the *address* of an object stored on the stack. – Jim Balter Jul 27 '13 at 03:40
  • @user2555139 You would be wise to ignore this answer which, aside from the first sentence, is quite wrong. There's nothing wrong with make_tauss8, which was written by the competent Boost C programmers, just with your usage of it. You can simply do `taus88_t TAUS88 = make_taus88(6346456);` -- no need for malloc. But you need `u32 numberu32 = taus88u32(&TAUS88); f32 numberf32 = taus88f32(&TAUS88);` – Jim Balter Jul 27 '13 at 03:54
  • @user2555139 Oops, that isn't Boost's version of make_tauss88; sorry about that. Still, I'm sure you didn't write it, and you shouldn't be changing it. – Jim Balter Jul 27 '13 at 04:12
  • @JimBalter I looked at the boost library for this first actually and I couldn't find any way to use an example for Taus88 so I ended up with this junk of code instead. meh! – pandoragami Jul 27 '13 at 19:37