Givaro's (3.2.13.rc1) C++ headers don't conform to C++11

[leif]

This breaks the build of its test suite as well as of the Sage library with (e.g.) GCC 4.7.x.

I already have an spkg fixing this, i.e., the offending headers.

See #12751 for the GCC-4.7.0 metaticket.

Closely related: #12444 (which I unfortunately wasn't aware of until now) for the clang port

---

New spkg: ​http://boxen.math.washington.edu/home/jdemeyer/spkg/givaro-3.2.13.p0.spkg

givaro-3.2.13.p0 (Jeroen Demeyer, 25 May 2012)

• #12761: Restore upstream sources to vanilla 3.2.13 (the previous src/ directory was some never-released CVS version between givaro-3.2.13.rc1 and givaro-3.2.13, but bootstrapped with a different automake).
• Remove gmp++.h.patch which is upstreamed (the old diff was wrong).
• Use patch to apply all patches.
• Fix patch for givtablelimits.h such that it can be applied on all systems, not only Cygwin.
• Merged all GCC-4.7.0 patches into one: cplusplus_scoping.patch
• Don't touch .pyx files, instead fix module_list.py (also on #12761).

givaro-3.2.13.rc1.p4 (Leif Leonhardy, March 27th 2012)

• #12761: Fix headers not conforming to C++11 to make Sage (especially the Sage library) build with GCC 4.7.0 (and without -fpermissive). Same for Givaro's test suite, which uses / instantiates much more! (These headers get installed into $SAGE_LOCAL/include/givaro/.) New patches:
  • patches/src.kernel.integer.givintnumtheo.inl.patch
  • patches/src.kernel.integer.givintrsa.inl.patch
  • patches/src.library.poly1.givpoly1factor.inl.patch
  • patches/src.library.poly1.givpoly1padic.h.patch
  • patches/src.library.poly1.givpoly1proot.inl.patch
• Remove the obsolete Debian dist/ directory.
• Remove obsolete GCC 4.3 patch.
• Rename diffs of prepatched files that are (still) copied over to *.diff (rather than *.patch) such that they don't get "automatically" applied by the patch -p1 loop, which I added.
• Fix permissions of SPKG.txt and spkg-install, and two upstream files.
• Add "Special Update/Build? Instructions" section.
• Clean up spkg-check and spkg-install.
• Also set up environment variables in spkg-check, as make check involves compilation. (Although configure should have put them into the generated Makefiles.)
• Use $MAKE in spkg-check as well.
• Exit in case the build failed!
• Only touch extension modules (*.pyx) if they (already) exist.

[leif]

Diff between the previous spkg in Sage and my new p4 spkg. For reference / review only.

[jdemeyer]

This is something which should be reported upstream (sending them the patch of course).

[jdemeyer]

If adding the -I flag is not necessary, then why add it while saying it is not needed?

# It shouldn't be necessary to add Sage's include directory here, 
# since we configure with '--with-gmp=...'. 
# Also, '-I...' should normally be added to (just) CPPFLAGS. 
CFLAGS="$CFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 
CXXFLAGS="$CXXFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 

[jdemeyer]

Don't Cython modules have proper dependency checking these days? Then the "touching" part might not be needed.

[leif]

Replying to jdemeyer:

If adding the -I flag is not necessary, then why add it while saying it is not needed?

# It shouldn't be necessary to add Sage's include directory here, 
# since we configure with '--with-gmp=...'. 
# Also, '-I...' should normally be added to (just) CPPFLAGS. 
CFLAGS="$CFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 
CXXFLAGS="$CXXFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 

I haven't added this, nor have I checked that it works, i.e., builds, without that (although I'm pretty sure it would) -- that's why I wrote "shouldn't be necessary", not "isn't needed".

One side effect of adding it is that the test suite is built with the headers installed into Sage, which isn't all bad, but I can try to remove it later, although quite unrelated to the purpose of this ticket.

W.r.t. touching: It doesn't hurt. Moreover, the installed headers might have timestamps earlier than those of the Python extension modules, such that Cython probably won't notice that they're "new".

[leif]

Replying to jdemeyer:

This is something which should be reported upstream (sending them the patch of course).

I cc'ed the spkg maintainers, which include Clément Pernet, according to SPKG.txt also the upstream contact.

Note that (as already mentioned elsewhere) the version currently in Sage is a release candidate, dated 2008 IIRC.

[leif]

Added a reference to #12444.

[ddrake]

The new spkg actually builds and passes tests (such as they are) on skynet/cicero. I can give a positive review to everything except your new header patches; they seem simple enough (almost all of it is just casting things to other types, right?) but I don't know enough C++ to be confident about that.

[leif]

Replying to ddrake:

The new spkg actually builds and passes tests (such as they are) on skynet/cicero. I can give a positive review to everything

Fine.

except your new header patches; they seem simple enough (almost all of it is just casting things to other types, right?) but I don't know enough C++ to be confident about that.

Nope, I haven't added casts, but (roughly speaking) qualified the names of (mostly, I think) member functions used in template class member functions, i.e.,

template <class Foo>
void Bar<Foo>::bar() // member function bar() of class Bar, which is parameterized by a class Foo
{
    Bar<Foo>::baz(); // call with qualified [function] name; 
                     // baz() here is [also] a member function of [template] class Bar
}

instead of

...
{
    baz(); // call with unqualified [function] name
}

I could have used

...
{
    this->baz();
}

instead in most (if not all) cases as well; cf. Andrew Ohana's changes at #12444 (which I unfortunately found only after I made my own changes).

[leif]

(Note that just using baz() wouldn't be a problem if it was already declared as a "direct" member function of Bar; problems arise if baz() isn't yet known at the point of the template definition, e.g. because it is inherited from a class which is a template parameter. I rather wanted to explain the Bar<Foo>:: prefix above.)

[Snark]

givaro-3.2.13.rc1.p3 from 5.0.rc1 built well already, but I still gave a try to : a84996518e39a1197eaf63562d8fe734 givaro-3.2.13.rc1.p4.spkg and it built well too on my debian x86_64 box using gcc 4.7.0.

[Snark]

Upstream latest version 3.5.0 doesn't compile with gcc 4.7 ; but their trunk does. I have written them to know when a new release will be available.

Did you discuss your patch with upstream?

[Snark]

Upstream just released 3.6.0 today -- just upgrade!

[jdemeyer]

Diff between leif's 3.2.13.rc1.p4 and my 3.2.13.p0 spkg. For reference / review only.

[kcrisman]

This works and passes tests on sage.math with GCC-4.7.0 and with some 4.6.x on Mac OS X, and all the changes make sense, so I think this is ready to go as well.

[Snark]

Why not just upgrade to upstream 3.6.0 ?