akrinke / Font-Stash

A dynamic font glyph cache for OpenGL.
114 stars 18 forks source link

couple of warnings fixed #2

Closed r-lyeh-archived closed 11 years ago

r-lyeh-archived commented 11 years ago

hey @akrinke,

I've fixed a couple of warnings in my fork. I'm using C++ VisualStudio 2012 compiler.

Cheers and keep up the good work!

akrinke commented 11 years ago

Thank you for the fixes! I have one question regarding stb_truetype.c: Why not just #define STBTT_assert(...) (i.e. an empty definition that simple removes all calls)?

r-lyeh-archived commented 11 years ago

Hey @akrinke, np! :)

Consider the following define:

define ASSIGN( var, value ) { var = value; printf("%d", var); }

Consider the following piece of code too.

a = 0; if( false ) ASSIGN( a, 1 ); else ASSIGN( a, 2 );

This can lead to following undesirable behaviours when expanded, ie:

if( false ) { { a = 1; printf("%d", a); } }; else { { a = 2; printf("%d", a); } };

Notice the dangling semicolon + else combination "; else". This won't compile. A simpler solution could be omitting the ; after any ASSIGN() macro, but code looks horrible this way:

if( false ) ASSIGN( a, 1 ) else ASSIGN( a, 2 )

Or encapsulating every ASSIGN() macro in brackets, which is a hard rule to remember too:

if( false ) { ASSIGN( a, 1 ); } else { ASSIGN( a, 2 ); }

So, the right and safer solution is to define the expression as:

define ASSIGN( a, value ) do { a = value; printf("%d", a); } while(0)

which gets expanded to:

if( false ) do { a = 1; printf("%d", a); } while(0); else do { a = 2; printf("%d", a); } while(0);

which is valid and will compile ok.

So, going back to the original question:

Personal taste! Both options should be equally valid:

define assert( expr ) /empty/

define assert( expr ) do { /empty/ } while(0)

I just use that second one as a possible skeleton to fill it up with code any time later. Say, just in case somebody is going to fill it up later with a bunch of instructions.

It is a good practise to me anyways. Most if not all compilers wont generate code for a empty syntax block like that.

Have a nice merging! :)

akrinke commented 11 years ago

Thank you very much for the detailed explanation! I scanned stb_truetype.h for uses of this macro and it was used as single command on a separate line in every case. That's why I was asking. But I agree, better safe than sorry.