Opened 5 years ago

Closed 5 years ago

#18323 closed enhancement (wontfix)

switch to C++11 when compiling some cython files

Reported by: rws Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords: c++ gcc cxx gcc5
Cc: Merged in:
Authors: Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: u/rws/18323-1 (Commits) Commit: 7fd25bc1eea60abb8f686d611bd40f69996d343f
Dependencies: #18450, C++11 compiler Stopgaps:

Description (last modified by leif)

C++11 is needed by a development version of Pynac, and GCC 5.x's g++ again got stricter w.r.t. C++ standard conformance (while it still defaults to -std=gnu++98).

Sage needs -std=c++11 for at least all files that import ginac headers, that is, for

src/sage/symbolic/constants_c.pyx
src/sage/symbolic/expression.pyx
src/sage/symbolic/function.pyx
src/sage/symbolic/getitem.pyx
src/sage/symbolic/pynac.pyx
src/sage/symbolic/ring.pyx
src/sage/symbolic/substitution_map.pyx

Change History (44)

comment:1 Changed 5 years ago by leif

Well, we don't have to compile everything in C++ (implicitly or explicitly) with -std=c++11, but Lcalc's special anyway (cf. http://trac.sagemath.org/ticket/12437#comment:15 for example). See also #12426 and many (already closed) tickets dealing with C++11/GCC 4.7, 5.x.

comment:2 Changed 5 years ago by leif

P.S.: While the GCC 4.6 and 4.7 series reached "end of life", there are still (LTS) distros using these.

Last edited 5 years ago by leif (previous) (diff)

comment:3 follow-up: Changed 5 years ago by fbissey

Remember also that gcc 5.1 is defaulting to -std=gnu11 for C so there may be other surprises but you would already have noticed them I am guessing?

comment:4 follow-up: Changed 5 years ago by fbissey

We should list all the other tickets for C++11 or have a meta ticket somewhere.

comment:5 in reply to: ↑ 3 Changed 5 years ago by leif

Replying to fbissey:

Remember also that gcc 5.1 is defaulting to -std=gnu11 for C so there may be other surprises but you would already have noticed them I am guessing?

Ahem, like #18326, which is a duplicate of #18247? B)

comment:6 in reply to: ↑ 4 Changed 5 years ago by leif

Replying to fbissey:

We should list all the other tickets for C++11 or have a meta ticket somewhere.

We've already done a lot in that direction in the past, see for example the closed metaticket on GCC 4.7.0 (#12751), or search for C++11 on trac.

Related: #12426 (Metaticket for Clang; at least some issues are related to C++ or GNUisms)

comment:7 follow-up: Changed 5 years ago by fbissey

I like meta tickets (but not abusing them). I guess I should rather look for a port to gcc 5.1 meta ticket, this one would just be a consequence. Looking at #18301now...

comment:8 in reply to: ↑ 7 Changed 5 years ago by leif

Replying to fbissey:

I like meta tickets (but not abusing them). I guess I should rather look for a port to gcc 5.1 meta ticket, this one would just be a consequence. Looking at #18301 now...

There's a thread "Issues with GCC 5.x" on sage-devel. Besides MPIR, Lcalc, and ncurses (all fixed), I'm having problems with gf2x on x86_64 (work-around: -O0 or GF2X_CONFIGURE=--disable-hardware-specific-code).

comment:9 Changed 5 years ago by rws

I just confirmed that you simply cannot compile a package with C++11 and expect the .so file to match all references generated by libsage compilation using a pre-C++11 compile. Specifically the template class std::vector<GiNaC::ex> will create incompatible code.

Okay, you knew that beforehand, but I had to prove it to myself.

comment:10 Changed 5 years ago by rws

  • Description modified (diff)
  • Summary changed from switch to C++11 to switch to C++11 when compiling some cython files

Sorry for specializing but only this guarantees that I will be working on it.

comment:11 Changed 5 years ago by rws

  • Branch set to u/rws/switch_to_c__11_when_compiling_some_cython_files

comment:12 Changed 5 years ago by git

  • Commit set to 9875e951d9f29e18b1dadef70989e92a27afbfc9

Branch pushed to git repo; I updated commit sha1. New commits:

9875e9518323: add extra C++11 compile flags for symbolics because of Pynac-0.4

comment:13 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Status changed from new to needs_review

comment:14 follow-up: Changed 5 years ago by fbissey

I'll be picky.

1) I would have put that patch in the same ticket that starts the use of C++11 in pynac. That's where it belongs, it's an associated change. 2) cosmetic, the indentation is wrong. 3) do you need to define a variable with an extra long name to replace what is just -std=c++11 in only three different places? It feels overkill. I would have tolerated a variable with a shorter name, to switch C++11 on - we'll see more of that certainly. Unless you plan to add more flags in the near future, I don't see why it should be symbolics specific

comment:15 Changed 5 years ago by rws

  • Milestone changed from sage-6.7 to sage-pending

Thanks for the input. We could as well wait until gcc5 is mandatory in Sage, or even earlier, as soon as all C++ packages compile with C++11, and then switch globally. It's not that there is pressing interest in Pynac right now.

comment:16 Changed 5 years ago by leif

Just for the record: g++ 5.x doesn't default to -std=c++11, not even -std=gnu++11 or -std=gnu++0x, but -std=gnu++98.

comment:17 Changed 5 years ago by leif

  • Description modified (diff)

comment:18 Changed 5 years ago by leif

... and the first g++ version said to be C++11 feature-complete is 4.8.1.

Since we already have 4.9.2 in Sage, we'd "just" have to SAGE_INSTALL_GCC on systems with 4.8.0 (if such exist at all), 4.7, 4.6, and earlier.

Ubuntu 12.04 with 5-year LTS still ships with "Ubuntu/Linaro? GCC" 4.6 for example.

comment:19 Changed 5 years ago by leif

If you have time, you could try building Sage with -Werror=c++11-compat in CXXFLAGS (CFLAGS for modules built by distutils, or added to extra_compile_args)... :P

comment:20 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You should leave module_list.py alone and move those flags to the appropriate .pxd file.

comment:21 in reply to: ↑ 20 Changed 5 years ago by rws

Replying to jdemeyer:

You should leave module_list.py alone and move those flags to the appropriate .pxd file.

How do I do that? The manual says nothing of it:

"A definition file can contain:

  • Any kind of C type declaration.
  • extern C function or variable declarations.
  • Declarations of C functions defined in the module.
  • The definition part of an extension type (see below)."

comment:22 Changed 5 years ago by jdemeyer

See #18450 (note that this ticket might also conflict with that)

comment:23 Changed 5 years ago by rws

  • Branch changed from u/rws/switch_to_c__11_when_compiling_some_cython_files to u/rws/18323

comment:24 follow-up: Changed 5 years ago by rws

  • Commit changed from 9875e951d9f29e18b1dadef70989e92a27afbfc9 to cf808d3d0d72233edb693e930b007eea1913c7ca
  • Dependencies set to C++11 compiler
  • Status changed from needs_work to needs_review

This works now but still relies on a C++11 compiler. It was discussed above what efforts were already done to make the rest of Sage gcc5 compatible. My mistake was to assume that with gcc5 we would automatically get C++11. So what are the chances that whatever shipped gcc, which is at 4.9x I understand, will be made mandatory (it need not be switched to C++11 globally, it just needs to be there when the seven files are compiled)?


New commits:

cf808d318323: switch to C++11 when compiling some cython files

comment:25 follow-up: Changed 5 years ago by jdemeyer

If those flags are only needed to support pynac, they should only be added to pynac declarations, not other random files like ring.pxd.

comment:26 Changed 5 years ago by jdemeyer

  • Dependencies changed from C++11 compiler to #18450, C++11 compiler
  • Status changed from needs_review to needs_work

comment:27 Changed 5 years ago by jdemeyer

This needs https://github.com/cython/cython/pull/381: therefore, it needs to depend on #18450 or you need to wait for the next Cython release.

comment:28 in reply to: ↑ 24 Changed 5 years ago by jdemeyer

Replying to rws:

So what are the chances that whatever shipped gcc, which is at 4.9x I understand, will be made mandatory (it need not be switched to C++11 globally, it just needs to be there when the seven files are compiled)?

I don't think we should ever make GCC 4.9.x mandatory. What could be discussed is making C++0x or C++11 support mandatory.

comment:29 Changed 5 years ago by rws

  • Branch changed from u/rws/18323 to u/rws/18323-1

comment:30 in reply to: ↑ 25 ; follow-ups: Changed 5 years ago by rws

  • Commit changed from cf808d3d0d72233edb693e930b007eea1913c7ca to 7fd25bc1eea60abb8f686d611bd40f69996d343f
  • Dependencies changed from #18450, C++11 compiler to C++11 compiler
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

If those flags are only needed to support pynac, they should only be added to pynac declarations, not other random files like ring.pxd.

Indeed, it boils down to expression.pxd.

it needs to depend on #18450 or you need to wait for the next Cython release.

It's pending, so just wait. I expect some friction with mandatory C++11, anyway.


New commits:

7fd25bc18323: switch to C++11 when compiling some cython files

comment:31 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by jdemeyer

Replying to rws:

Indeed, it boils down to expression.pxd.

Really? Not ginac.pxd?

comment:32 in reply to: ↑ 31 Changed 5 years ago by rws

Replying to jdemeyer:

Replying to rws:

Indeed, it boils down to expression.pxd.

Really? Not ginac.pxd?

Never had a look at that: both work because the same files are affected. Granted, it would fit better with ginac.pxd.

comment:33 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:34 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by jdemeyer

  • Dependencies changed from C++11 compiler to #18450, C++11 compiler

Replying to rws:

I expect some friction with mandatory C++11, anyway.

Personally, I don't think it would be such a problem. The most annoying part will be testing with older GCC versions (for example, GCC 4.6.x supports some C++11 features, are these enough for Pynac?).

I put back the dependency on #18450 since otherwise this patch won't work properly.

comment:35 in reply to: ↑ 34 Changed 5 years ago by rws

Replying to jdemeyer:

Replying to rws:

I expect some friction with mandatory C++11, anyway.

Personally, I don't think it would be such a problem. The most annoying part will be testing with older GCC versions (for example, GCC 4.6.x supports some C++11 features, are these enough for Pynac?).

From a table I have here 4.6 is missing override. I could hold back usage of override, no problem.

comment:36 Changed 5 years ago by fbissey

Coming back to this... We could check for the feature that are necessary. If you need override we can first check for c++11 as suggested by Jeroen, once we know that we have a c++11 switch we can check for particular features like override it is just a matter of writing a test in configure.ac.

But making a decision on what we should want as minimal features for the compiler should be matter of debate on sage-devel.

comment:37 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jdemeyer

  • Authors Ralf Stephan deleted
  • Milestone changed from sage-pending to sage-duplicate/invalid/wontfix
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to needs_review

Replying to fbissey:

I would have put that patch in the same ticket that starts the use of C++11 in pynac. That's where it belongs, it's an associated change.

I totally agree. Close as "wontfix"?

comment:38 in reply to: ↑ 37 Changed 5 years ago by fbissey

  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Replying to fbissey:

I would have put that patch in the same ticket that starts the use of C++11 in pynac. That's where it belongs, it's an associated change.

I totally agree. Close as "wontfix"?

I am fine with closing. We should open a new ticket when we have decided exactly what we want on sage-devel.

comment:39 Changed 5 years ago by vbraun

I disagree. Holding back on allowing C++11 just means

  • taking on more technical debt as authors can't take advantage of the new features, writing code that may have to be changed later
  • prevent contributors from being exposed to new C++11 features, slowing down their learning
  • online C++ resources increasingly include C++11 features, making it even more difficult to learn how to not use them
  • delaying the inevitable, we'll have to shitlist broken compilers anyways

comment:40 follow-ups: Changed 5 years ago by jdemeyer

This ticket is about adding std=c++11 for some ginac files. It's not about C++11 in general.

Besides, why do you think that people are prevented from using C++11? It's not like we prevent people from using their own C++11 compiler. Once their code gets in Sage, we make the switch in Sage.

comment:41 in reply to: ↑ 40 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

This ticket is about adding std=c++11 for some ginac files. It's not about C++11 in general.

And a separate ticket wasn't the right place for it.

Besides, why do you think that people are prevented from using C++11? It's not like we prevent people from using their own C++11 compiler. Once their code gets in Sage, we make the switch in Sage.

This ticket is not the right forum for the discussion.

Nevertheless which minimum gcc would we be aiming for?

comment:42 in reply to: ↑ 41 Changed 5 years ago by jdemeyer

Replying to fbissey:

This ticket is not the right forum for the discussion.

Right, there is already a thread on sage-devel. Let's just close this discussion here.

comment:43 in reply to: ↑ 40 Changed 5 years ago by cpernet

Replying to jdemeyer:

Once their code gets in Sage, we make the switch in Sage.

We are facing this problem for the update of LinBox Givaro and fflas-ffpack in #17635.

comment:44 Changed 5 years ago by vbraun

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.