wdas / SeExpr

SeExpr is an embeddable, arithmetic expression language that enables flexible artistic control and customization in creating computer graphics images. Example uses include procedural geometry synthesis, image synthesis, simulation control, crowd animation, and geometry deformation. https://wdas.github.io/SeExpr
https://www.disneyanimation.com/open-source/seexpr/
Other
405 stars 86 forks source link

SePlatform.h should not be installed or not redefine system stuff #33

Open devernay opened 8 years ago

devernay commented 8 years ago

SePlatform.h is included by SeExprBuiltins.h, and installed with the other includes, but it does a few pretty bad things on the windows platform, such as:

#   define snprintf sprintf_s

(which totally breaks snprintf if compiling with mingw for example)

A public library include should not do such things (we had a hard time figuring out what was happening).

There are two alternatives: 1- do not include SePlatform.h in SeExprBuiltins.h, do not install it, and make sure the SeExpr public includes work without it. 2- leave all the #defines out of SePlatform.h and define these in a private include (e.g. SePrivate.h)

sopvop commented 8 years ago

That's how this is fixed in another library https://github.com/akheron/jansson/commit/db0213ae561156611eaa54dfeac0d6684c2f1883

devernay commented 8 years ago

thanks for the link @sovpop , except I don't like that part:

#  else  /* Other Windows compiller, old definition */
#    define snprintf _snprintf
#    define vsnprintf _vsnprintf
#  endif

because other compilers (e.g. mingw) may provide a proper implementation of snprintf. That's precisely what happens with MinGW.

And in any case, this should not be done in public headers.

aselle commented 8 years ago

We can take a look at fixing this. One other option would be to not use sprintf directly and instead make a function sePrintf() that is defined in a cpp so none of the platform specific stuff gets exported. I.e. we should probably not be defining things to things that might conflict with real functions... So...

define SeExprSprintf sprintf

kind of defines might be better. I'll admit I haven't tried to build seexpr on windows with mingw32