Bug 24318

Summary: Fix building of GLSL demos which use M_PI when M_PI is not defined by math.h
Product: Mesa Reporter: Jon TURNEY <jon.turney>
Component: DemosAssignee: mesa-dev
Status: REOPENED --- QA Contact:
Severity: minor    
Priority: medium    
Version: git   
Hardware: x86 (IA32)   
OS: Cygwin   
Whiteboard:
i915 platform: i915 features:
Attachments: Patch to fix building of GLSL demos which use M_PI when M_PI is not defined by math.h
Remove the various definitions of M_PI, except in compiler.h
Add an autoconf check that math.h defines M_PI
Add an autoconf check that math.h defines M_PI
Remove the various definitions of M_PI, except in compiler.h
Remove the various definitions of M_PI

Description Jon TURNEY 2009-10-05 06:16:28 UTC
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.
Comment 1 Brian Paul 2009-10-05 10:09:25 UTC
Committed to the 7.6 branch. Thanks.
Comment 2 Ian Romanick 2009-10-05 10:18:33 UTC
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.
Comment 3 Jon TURNEY 2009-10-05 10:34:51 UTC
(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 
Comment 4 Brian Paul 2009-10-05 10:38:27 UTC
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.

Comment 5 Jon TURNEY 2009-10-05 11:02:33 UTC
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
Comment 6 Tom Fogal 2009-10-05 11:12:15 UTC
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
Comment 7 Jon TURNEY 2009-10-07 04:31:13 UTC
Reopening so I don't forget about this
Comment 8 Matt Turner 2009-10-07 06:37:45 UTC
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'
Comment 9 Jon TURNEY 2009-10-24 10:17:59 UTC
Created attachment 30659 [details] [review]
Remove the various definitions of M_PI, except in compiler.h
Comment 10 Jon TURNEY 2009-10-24 10:18:46 UTC
Created attachment 30660 [details] [review]
Add an autoconf check that math.h defines M_PI
Comment 11 Jon TURNEY 2009-10-24 10:24:29 UTC
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.

Comment 12 Jon TURNEY 2010-04-26 11:51:38 UTC
Created attachment 35298 [details] [review]
Add an autoconf check that math.h defines M_PI
Comment 13 Jon TURNEY 2010-04-26 11:52:03 UTC
Created attachment 35299 [details] [review]
Remove the various definitions of M_PI, except in compiler.h
Comment 14 Jon TURNEY 2010-04-26 11:53:49 UTC
(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
Comment 15 Jon TURNEY 2010-07-16 17:57:46 UTC
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.