Visual C++

[davegorst]

Hiya,

I had a go at forking this this morning to make it play nicely with Visual C++, which while it's reasonably up to date with C++ is definitely stuck in the last century when it comes to C (it's still shy of C99). I had a go at making of it ANSI C friendly too while I was there.Seems to work OK from what I can tell.

Best regards, Dave.

[smcameron]

Hi, and thanks. It will be this evening (at the soonest) before I can take a look at this. I did notice one thing... where you typedef char int8_t, and so on there's a comment that says those are guesses, may have to tweak, etc. You could catch it when it goes wrong at compile time via a macro like the BUILD_ASSERT macro that's here:

https://github.com/rustyrussell/stats/blob/master/ccan/build_assert/build_assert.h

-- steve

[davegorst]

Hi,

That's neat. To be honest I'd actually forgotten stdint.h wasn't in C89 so that was a bit of an afterthought on the train - I was thinking of adding similar checks but thought that no-one could still be using 8 or 16 bit systems today could they? And if they do 64 bit int's are unlikely to be supported - it was only really 32-bit int's I was worrying about and there aren't any.

There were also another couple of bits that I couldn't quite remember (was it K&R or ANSI that required you to only assign an initial value to local variables after all the uninitialised ones?), but gcc seems happy enough with the "-ansi" switch, and MSVC too.

Will add some checks tomorrow anyway (Australian time so will be a while) - maybe they should go in the test code.

Cheers, Dave.

On 10 September 2015 at 00:29, smcameron notifications@github.com wrote:

Hi, and thanks. It will be this evening (at the soonest) before I can take a look at this. I did notice one thing... where you typedef char int8_t, and so on there's a comment that says those are guesses, may have to tweak, etc. You could catch it when it goes wrong at compile time via a macro like the BUILD_ASSERT macro that's here:

https://github.com/rustyrussell/stats/blob/master/ccan/build_assert/build_assert.h

-- steve

— Reply to this email directly or view it on GitHub https://github.com/smcameron/open-simplex-noise-in-c/pull/7#issuecomment-138927310 .

[davegorst]

Hi again,

I put those checks in first thing this morning and committed. They seem to work well.

Nobody at work seems to remember the thing about variables, so it's possible I've either remembered wrongly or it was due to one of the terrible compilers I was using at the time. I'll leave that one for now.

Best regards, Dave.

On 10 September 2015 at 01:35, Dave Gorst dgorst@gmail.com wrote:

Hi,

That's neat. To be honest I'd actually forgotten stdint.h wasn't in C89 so that was a bit of an afterthought on the train - I was thinking of adding similar checks but thought that no-one could still be using 8 or 16 bit systems today could they? And if they do 64 bit int's are unlikely to be supported - it was only really 32-bit int's I was worrying about and there aren't any.

There were also another couple of bits that I couldn't quite remember (was it K&R or ANSI that required you to only assign an initial value to local variables after all the uninitialised ones?), but gcc seems happy enough with the "-ansi" switch, and MSVC too.

Will add some checks tomorrow anyway (Australian time so will be a while)

• maybe they should go in the test code.

Cheers, Dave.

On 10 September 2015 at 00:29, smcameron notifications@github.com wrote:

Hi, and thanks. It will be this evening (at the soonest) before I can take a look at this. I did notice one thing... where you typedef char int8_t, and so on there's a comment that says those are guesses, may have to tweak, etc. You could catch it when it goes wrong at compile time via a macro like the BUILD_ASSERT macro that's here:

https://github.com/rustyrussell/stats/blob/master/ccan/build_assert/build_assert.h

-- steve

— Reply to this email directly or view it on GitHub https://github.com/smcameron/open-simplex-noise-in-c/pull/7#issuecomment-138927310 .

[smcameron]

Hi,

A few more comments...

-   ctx->permGradIndex3D = malloc(sizeof(*ctx->permGradIndex3D) * ngrad);
+   ctx->permGradIndex3D = (int16_t *) malloc(sizeof(*ctx->permGradIndex3D) * ngrad);

Why do you need that cast? As long as malloc is declared to return void * (as it is via stdlib.h, iirc) and not implicitly declared to return int, then this cast should not be needed, and may hide problems if malloc isn't declared explicitly on arch's where sizeof(int) != sizeof(void *). (I seem to recall somewhere needing to #include , but I think that was on some old mac or something? Or may have been wrong about that.) C++ needs a cast from malloc, but C doesn't, as far as I know.

+ void check_type_sizes (void)
+{
+       BUILD_ASSERT (sizeof (int8_t) == 1);
+       BUILD_ASSERT (sizeof (int16_t) == 2);
+       BUILD_ASSERT (sizeof (int64_t) == 8);
+}

This unused function is polluting the global namespace. Making it static will make gcc complain about it being declared but not used, so maybe something like this?

__attribute__((unused)) static void check_type_sizes(void)

with maybe some macro cruft to encapsulate the attribute((unused)) gcc-ism? Or, better yet, just put those build asserts into another function (they do not generate any code.)

+   check_type_sizes();

You don't need to call the function. The check happens entirely at compile time with no code generated. The trick it uses, iirc, is to declare an anonymous type that is an array of a positive dimension (1) if the assert passes and an anonymous type that is array of size -1 if it fails -- the size of -1 won't compile, which is what you want when the assert fails. (It also takes the size of the anonymous type, because a bare int is a legal statement in C -- a statement that has no effect, and generates no code. So a lot of clever trickery to get that check to happen in the compiler.)

+#if __STDC_VERSION__ >= 199901L
+   #define STIN static inline
+#elif MSC_VER
+   #define STIN static __inline
+#else
+   /* ANSI C doesn't have inline. */
+   #define STIN
+#endif
+

Does ANSI C have static? I kind of think it does. So I think "static" isn't a problem, it's just the "inline", right? So how about define INLINE as either inline or __inline, or as nothing, and leave the "static" part alone? then static INLINE void blah()...

-- steve

[davegorst]

Hiya,

It actually errors in Visual Studio without the casts when compiling as C++ rather than C, which is why I thought it worth adding. I think it's cast to a pointer (int16_t *) rather than int16_t? I have :

• ctx->permGradIndex3D = malloc(sizeof(ctx->permGradIndex3D) \ ngrad);
• ctx->permGradIndex3D = (int16t ) malloc(sizeof(_ctx->permGradIndex3D) * ngrad);

I take your point about putting the asserts into another function in the test harness code. It would seem to make more sense, and would avoid the GCC'isms, so it's now at the start of main(). fastfloor is now static again too.

Dave.

On 10 September 2015 at 16:33, smcameron notifications@github.com wrote:

Hi,

A few more comments...

• ctx->permGradIndex3D = malloc(sizeof(ctx->permGradIndex3D) \ ngrad);
• ctx->permGradIndex3D = (int16t ) malloc(sizeof(_ctx->permGradIndex3D)
• ngrad);

Why do you need that cast? As long as malloc is declared to return void (as it is via stdlib.h, iirc) and not implicitly declared to return int, then this cast should not be needed, and may hide problems if malloc isn't declared explicitly on arch's where sizeof(int) != sizeof(void ). (I seem to recall somewhere needing to #include , but I think that was on some old mac or something? Or may have been wrong about that.) C++ needs a cast from malloc, but C doesn't, as far as I know.

• void check_type_sizes (void) +{
• BUILD_ASSERT (sizeof (int8_t) == 1);
• BUILD_ASSERT (sizeof (int16_t) == 2);
• BUILD_ASSERT (sizeof (int64_t) == 8); +}

This unused function is polluting the global namespace. Making it static will make gcc complain about it being declared but not used, so maybe something like this?

attribute((unused)) static void check_type_sizes(void)

with maybe some macro cruft to encapsulate the attribute((unused)) gcc-ism? Or, better yet, just put those build asserts into another function (they do not generate any code.)

• check_type_sizes();

You don't need to call the function. The check happens entirely at compile time with no code generated. The trick it uses, iirc, is to declare an anonymous type that is an array of a positive dimension (1) if the assert passes and an anonymous type that is array of size -1 if it fails -- the size of -1 won't compile, which is what you want when the assert fails. (It also takes the size of the anonymous type, because a bare int is a legal statement in C -- a statement that has no effect, and generates no code. So a lot of clever trickery to get that check to happen in the compiler.)

+#if _STDCVERSION >= 199901L

• define STIN static inline +#elif MSC_VER

• define STIN static __inline +#else

• /* ANSI C doesn't have inline. */

• define STIN +#endif +

Does ANSI C have static? I kind of think it does. So I think "static" isn't a problem, it's just the "inline", right? So how about define INLINE as either inline or __inline, or as nothing, and leave the "static" part alone? then static INLINE void blah()...

-- steve

— Reply to this email directly or view it on GitHub https://github.com/smcameron/open-simplex-noise-in-c/pull/7#issuecomment-139130198 .

[smcameron]

Well, yes, I imagine it would complain about those casts when compiled as if it were C++. :) Alright, leave the casts in, though it pains me.

I'm not sure what you mean about the int16_t vs. int16_t * <-- supposed to be a star here... maybe markdown is eating them?

I just gave you push access on my repo.

-- steve

[davegorst]

Yeah - something strange going on there!

Thanks - will test again on MSVC and Linux before I push anything up.

Cheers, Dave.

On 10 September 2015 at 17:26, smcameron notifications@github.com wrote:

Well, yes, I imagine it would complain about those casts when compiled as if it were C++. :) Alright, leave the casts in, though it pains me.

I'm not sure what you mean about the int16_t vs. int16_t * <-- supposed to be a star here... maybe markdown is eating them?

I just gave you push access on my repo.

-- steve

— Reply to this email directly or view it on GitHub https://github.com/smcameron/open-simplex-noise-in-c/pull/7#issuecomment-139137697 .

[davegorst]

I've done a bit of testing across different compilers with different standards set and I've added a check in for GNU's pre-C99 inline extension. I think I might get rid of the typedefs - they're likely to be more trouble than they're worth, as even if the compiler is really old they're likely to get pulled in by stdlib.h or types.h or whatever, and if they're missing we'll soon know about it anyway.

Cheers, Dave.

On 10 September 2015 at 17:31, Dave Gorst dgorst@gmail.com wrote:

Yeah - something strange going on there!

Thanks - will test again on MSVC and Linux before I push anything up.

Cheers, Dave.

On 10 September 2015 at 17:26, smcameron notifications@github.com wrote:

Well, yes, I imagine it would complain about those casts when compiled as if it were C++. :) Alright, leave the casts in, though it pains me.

I'm not sure what you mean about the int16_t vs. int16_t * <-- supposed to be a star here... maybe markdown is eating them?

I just gave you push access on my repo.

-- steve

— Reply to this email directly or view it on GitHub https://github.com/smcameron/open-simplex-noise-in-c/pull/7#issuecomment-139137697 .