Created attachment 30074 [details] [review] Patch to fix building of GLSL demos which use M_PI when M_PI is not defined by math.h Some <math.h> files do not define M_PI, in which case, we must provide our own definition. This is done for most demos, but not for the GLSL demos.
Committed to the 7.6 branch. Thanks.
NAK!!! The only systems where I have seen M_PI not defined is MSVC. On MSVC it is not defined /by default/, but it can be enabled by defining _USE_MATH_DEFINES. It would be much better to define _USE_MATH_DEFINES from the build system than pollute the code with this cruft.
(In reply to comment #2) > NAK!!! The only systems where I have seen M_PI not defined is MSVC. On MSVC > it is not defined /by default/, but it can be enabled by defining > _USE_MATH_DEFINES. It would be much better to define _USE_MATH_DEFINES from > the build system than pollute the code with this cruft. I'm building with gcc under Cygwin. Your point is well made, but... $ grep -r "#define M_PI" * | wc -l 48
Just a historical note: I recall needing the #ifndef M_PI stuff many years ago on some Unix variants (HP-UX?), before Mesa was running on Windows. If you want to revert the patch and fix the compile flags, that's OK w/ me. As Jon points out, a lot of other demos do this too.
I'll see if I can cook up a patch to do this 'properly' (or at least 'better') For the compiler/header combination I have, -std=c99 -U__STRICT_ANSI__ seems to be needed to let math.h defined M_PI
bugzilla-daemon@freedesktop.org writes: > http://bugs.freedesktop.org/show_bug.cgi?id=24318 > > --- Comment #2 from Ian Romanick <idr@freedesktop.org> 2009-10-05 > > NAK!!! The only systems where I have seen M_PI not defined is > MSVC. On MSVC it is not defined /by default/, but it can be enabled > by defining _USE_MATH_DEFINES. It would be much better to define > _USE_MATH_DEFINES from the build system than pollute the code with > this cruft. Not that using _USE_MATH_DEFINES sounds like a bad idea, but I don't see M_PI mentioned in the n1336 draft. I'm not sure where I got n1336, but probably somewhere here: http://www.open-std.org/jtc1/sc22/wg14/ Thus, I would argue that *both* this and the build system define should be used. -tom
Reopening so I don't forget about this
IIRC when compiling with gcc -ansi, M_PI isn't defined. In my math.h, its definition is wrapped in '#if defined __USE_BSD || defined __USE_XOPEN'
Created attachment 30659 [details] [review] Remove the various definitions of M_PI, except in compiler.h
Created attachment 30660 [details] [review] Add an autoconf check that math.h defines M_PI
Attached a couple of patches with an effort to do this better. As written, this falls back to explicitly defining M_PI using -D o the compiler command line if it can't make math.h define it. This probably isn't a good idea. Probably it should do nothing, but this probably breaks stuff on some targets which should include compiler.h but doesn't? Visual C projects will need updating to use _USE_MATH_DEFINES. I have assumed that the precision of the value of M_PI is not significant, as it varies from place to place, which should be the case anyhow, as these values are only a fall-back (In reply to comment #8) > IIRC when compiling with gcc -ansi, M_PI isn't defined. > > In my math.h, its definition is wrapped in '#if defined __USE_BSD || defined > __USE_XOPEN' I have the same. I believe that -U__STRICT_ANSI__ is the way to access these definitions if you want to use -ansi, but this may be incorrect.
Created attachment 35298 [details] [review] Add an autoconf check that math.h defines M_PI
Created attachment 35299 [details] [review] Remove the various definitions of M_PI, except in compiler.h
(In reply to comment #11) > Attached a couple of patches with an effort to do this better. Refreshed patches for git master > As written, this falls back to explicitly defining M_PI using -D o the compiler > command line if it can't make math.h define it. This probably isn't a good > idea. Changed this so ./configure will fail with an error if we can't work out how to get math.h to define M_PI
Created attachment 37144 [details] [review] Remove the various definitions of M_PI Updated patch for separate mesa-demos git respository. The autoconf changes don't seem to be required anymore as the gcc flag -ansi is no longer used, so M_PI is defined.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/demos/issues/2.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.