Opened 8 years ago

Closed 8 years ago

#10260 closed defect (fixed)

module_list.py: quaternion_algebra_element.pyx wrongly uses -std=c99 option

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-4.6.1
Component: build Keywords:
Cc: Merged in: sage-4.6.1.alpha2
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

It's C++ code...

Attachments (1)

10260_quaternion_algebra_element_cpp.patch (948 bytes) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by jdemeyer

comment:1 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-up: Changed 8 years ago by fbissey

Tell you what. I introduced exactly the same patch in sage-on-gentoo at the end of June. While you are at it can we fix this one as well:

    Extension('sage.rings.polynomial.polynomial_rational_flint',
              sources = ['sage/rings/polynomial/polynomial_rational_flint.pyx', 'sage/libs/flint/fmpq_poly.c'],
              language = 'c++',
              extra_compile_args=["-std=c99"] + uname_specific('SunOS', [], ['-D_XPG6']),
              libraries = ["csage", "flint", "ntl", "gmpxx", "gmp"],
              include_dirs = [SAGE_ROOT + '/local/include/FLINT/', SAGE_ROOT + '/devel/sage/sage/libs/flint/'],
              depends = [SAGE_ROOT + "/local/include/FLINT/flint.h"]),

That's the only other with that "QA" problem. I'll happily give it a positive review.

comment:3 follow-up: Changed 8 years ago by robertwb

Could you explain what "D_XPG6" is and why it's needed? (I've seen this elsewhere, and yet to have anyone justify it.)

comment:4 in reply to: ↑ 3 Changed 8 years ago by fbissey

Replying to robertwb:

Could you explain what "D_XPG6" is and why it's needed? (I've seen this elsewhere, and yet to have anyone justify it.)

After a quick googling it looks like something ugly. First hit says: XPG was the X/Open Portability Guide, a pre-POSIX consortium to promote interoperability between different UNIX vendors. XPG6 is "X/Open Portability Guide, Issue 6".

But that one is a little bit more verbose about what that flag is supposed to do: http://unix.derkeiler.com/Newsgroups/comp.unix.solaris/2007-01/msg00360.html In short make C99 code work inside c++ code... Which may explain why someone bothered to put it together with -std=c99. Also the post ask how to enable it on solaris which explains why there is a uname_specific command with it.

It seem to be regarded as a bad idea by a number of people.

comment:5 Changed 8 years ago by fbissey

  • Status changed from needs_review to positive_review

I am giving the present patch a positive review. It is fairly simple and as actually been in use in sage-on-gentoo for a while.

We can do the other one later in another ticket.

comment:6 in reply to: ↑ 2 Changed 8 years ago by jdemeyer

Replying to fbissey:

Tell you what. I introduced exactly the same patch in sage-on-gentoo at the end of June. While you are at it can we fix this one as well:

    Extension('sage.rings.polynomial.polynomial_rational_flint',
              sources = ['sage/rings/polynomial/polynomial_rational_flint.pyx', 'sage/libs/flint/fmpq_poly.c'],
              language = 'c++',
              extra_compile_args=["-std=c99"] + uname_specific('SunOS', [], ['-D_XPG6']),
              libraries = ["csage", "flint", "ntl", "gmpxx", "gmp"],
              include_dirs = [SAGE_ROOT + '/local/include/FLINT/', SAGE_ROOT + '/devel/sage/sage/libs/flint/'],
              depends = [SAGE_ROOT + "/local/include/FLINT/flint.h"]),

This one is harder, because there are two files, one C99 and one C++. So we cannot simply remove -std=c99 there.

comment:7 Changed 8 years ago by jdemeyer

  • Reviewers set to François Bissey

comment:8 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.