Opened 3 years ago

Closed 14 months ago

#23341 closed enhancement (fixed)

port lcalc to C++11

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.5
Component: number theory Keywords: lcalc
Cc: fbissey Merged in:
Authors: Dima Pasechnik, François Bissey Reviewers: Dima Pasechnik, François Bissey, Jeroen Demeyer, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 7cf967f (Commits) Commit: 7cf967fa5f50daf67e9a910786970bf184c37ecb
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

lcalc doesn't use standard C++:

  • using namespace std; needs an include with stuff from std:: before it

  • typeof is a gcc extension - C++11 has decltype instead

  • plain C must not be compiled with C++ compiler - thus a makefile change

Change History (93)

comment:1 Changed 3 years ago by fbissey

Can you rebase on 8.0.rc0? Your branch shows up in red.

comment:2 Changed 3 years ago by dimpase

this is needed for building lcalc with -std=c++11. (tested so far with clang 4.0.1 on Linux, with stdlib=libc++)

Only the top commit is of interest. Perhaps it can be based on rc0 indeed. But I need sleep now :-)

comment:3 follow-ups: Changed 3 years ago by fbissey

OK :)

The next question is how to ensure that the compiler then uses c++11. lcalc doesn't have a configure script so we cannot do it there. At the very least spkg-install will need to insert -std=c++11 in CXXFLAGS to make sure you are always using c++11.

comment:4 Changed 3 years ago by git

  • Commit changed from 015955efe9577dc9c3d443a4328297cd097454ee to 14e08ae6eb3f2d5c85f8b1e307ac5c1160d224ee

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

5434c70Merge branch 'develop' into compiler
f565739bump config tarball
53f9c4dMerge branch 'compiler' into clang-progress
7a9ab6bchecksum for this branch's configure tarball
14e08aeMerge branch 'u/fbissey/clang-progress' of trac.sagemath.org:sage into clang

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

the other problem here is an incorrect building of lcalc extension. Here C++ is compiled with a C compiler, which is patently wrong. Any idea how this can be fixed?

[sagelib-8.0.rc0] [  4/323] clang -fno-strict-aliasing -OPT:Olimit=0 -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -I/mnt/opt/Sage/sage-clang/local/lib/python2.7/site-packages/cypari2 -I/mnt/opt/Sage/sage-clang/local/lib/python2.7/site-packages/cysignals -I/mnt/opt/Sage/sage-clang/local/include -I/mnt/opt/Sage/sage-clang/local/include/python2.7 -I/mnt/opt/Sage/sage-clang/local/lib/python2.7/site-packages/numpy/core/include -I/mnt/opt/Sage/sage-clang/src -I/mnt/opt/Sage/sage-clang/src/sage/ext -I/mnt/opt/Sage/sage-clang/src/build/cythonized -I/mnt/opt/Sage/sage-clang/src/build/cythonized/sage/ext -I/mnt/opt/Sage/sage-clang/local/include/python2.7 -c /mnt/opt/Sage/sage-clang/src/build/cythonized/sage/libs/lcalc/lcalc_Lfunction.cpp -o build/temp.linux-x86_64-2.7/mnt/opt/Sage/sage-clang/src/build/cythonized/sage/libs/lcalc/lcalc_Lfunction.o -fno-strict-aliasing -O3 -ffast-math
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/src/build/cythonized/sage/libs/lcalc/lcalc_Lfunction.cpp:505:
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/src/sage/libs/lcalc/lcalc_sage.h:1:
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/local/include/libLfunction/L.h:34:
[sagelib-8.0.rc0] In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/6.3.0/include/g++-v6/backward/strstream:50:
[sagelib-8.0.rc0] /usr/lib/gcc/x86_64-pc-linux-gnu/6.3.0/include/g++-v6/backward/backward_warning.h:32:2: warning: This file includes at least one deprecated or antiquated header which   may be removed without further notice at a future date. Please use a   non-deprecated interface with equivalent functionality instead. For a   listing of replacement headers and interfaces, consult the file   backward_warning.h. To disable this warning use -Wno-deprecated. [-W#warnings]
[sagelib-8.0.rc0] #warning \
[sagelib-8.0.rc0]  ^
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/src/build/cythonized/sage/libs/lcalc/lcalc_Lfunction.cpp:505:
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/src/sage/libs/lcalc/lcalc_sage.h:1:
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/local/include/libLfunction/L.h:40:
[sagelib-8.0.rc0] In file included from /mnt/opt/Sage/sage-clang/local/include/libLfunction/Lglobals.h:48:
[sagelib-8.0.rc0] /mnt/opt/Sage/sage-clang/local/include/libLfunction/Lcommon.h:71:8: error: expected ';' in 'for' statement specifier
[sagelib-8.0.rc0]                 loop(i,this->N,N) coeffs[i]=zero;
[sagelib-8.0.rc0                       ^
...

comment:6 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by dimpase

Replying to fbissey:

OK :)

The next question is how to ensure that the compiler then uses c++11. lcalc doesn't have a configure script so we cannot do it there. At the very least spkg-install will need to insert -std=c++11 in CXXFLAGS to make sure you are always using c++11.

We may assume that all the C++ compilers we care about support -std=c++11, right? So this may be an independent of clang tickets thing?

comment:7 Changed 3 years ago by dimpase

  • Cc jdemeyer added
  • Component changed from PLEASE CHANGE to packages: standard
  • Type changed from PLEASE CHANGE to defect

Jeroen, do you know to force the use of C++ compiler here (with proper use of CXXFLAGS, too) ?

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

  • Cc jdemeyer removed
  • Component changed from packages: standard to PLEASE CHANGE
  • Type changed from defect to PLEASE CHANGE

Replying to dimpase:

the other problem here is an incorrect building of lcalc extension. Here C++ is compiled with a C compiler, which is patently wrong. Any idea how this can be fixed?

Yes. This is https://bugs.python.org/issue1222585 the patch needed for that is included in the gentoo patch tarball for python.

comment:9 in reply to: ↑ 6 Changed 3 years ago by fbissey

Replying to dimpase:

We may assume that all the C++ compilers we care about support -std=c++11, right? So this may be an independent of clang tickets thing?

Definitely. This shouldn't depend on #12426.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by dimpase

Replying to fbissey:

Replying to dimpase:

the other problem here is an incorrect building of lcalc extension. Here C++ is compiled with a C compiler, which is patently wrong. Any idea how this can be fixed?

Yes. This is https://bugs.python.org/issue1222585 the patch needed for that is included in the gentoo patch tarball for python.

Is there a Sage ticket for this already?

comment:11 in reply to: ↑ 10 Changed 3 years ago by fbissey

Replying to dimpase:

Replying to fbissey:

Replying to dimpase:

the other problem here is an incorrect building of lcalc extension. Here C++ is compiled with a C compiler, which is patently wrong. Any idea how this can be fixed?

Yes. This is https://bugs.python.org/issue1222585 the patch needed for that is included in the gentoo patch tarball for python.

Is there a Sage ticket for this already?

I don't think so. But it comes up regularly.

comment:12 follow-up: Changed 3 years ago by jdemeyer

How urgent is this? It might be possible to drop the lcalc package completely in favour of PARI/GP. However, the timeframe for this might be in the order on months (mainly because I don't know enough mathematically about L-functions).

comment:13 in reply to: ↑ 12 ; follow-ups: Changed 3 years ago by fbissey

Replying to jdemeyer:

How urgent is this? It might be possible to drop the lcalc package completely in favour of PARI/GP. However, the timeframe for this might be in the order on months (mainly because I don't know enough mathematically about L-functions).

I think Dima started this with the view of compiling sage with -std=c++11. If that's what he is doing, getting https://bugs.python.org/issue1222585 in would be more pressing in my opinion.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by dimpase

Replying to fbissey:

Replying to jdemeyer:

How urgent is this? It might be possible to drop the lcalc package completely in favour of PARI/GP. However, the timeframe for this might be in the order on months (mainly because I don't know enough mathematically about L-functions).

I think Dima started this with the view of compiling sage with -std=c++11.

indeed, and as a part of the clang porting effort too - it's just too much trouble to have different packages with different c++xy IMHO, especially if one depends on the another, they better be on the same xy...

If that's what he is doing, getting https://bugs.python.org/issue1222585 in would be more pressing in my opinion.

I'll see how it flies with these patches, and open a ticket.

comment:15 in reply to: ↑ 14 Changed 3 years ago by fbissey

Replying to dimpase:

I'll see how it flies with these patches, and open a ticket.

For info Gentoo's python includes those. sage-on-gentoo has been compiled using these patches to distutils for almost as long as it existed.

comment:16 Changed 3 years ago by dimpase

For the life of me, I don't understand why the patch 21_all_distutils_c++.patch from https://dev.gentoo.org/~floppym/python/python-gentoo-patches-2.7.13-0.tar.xz does not apply if I place it in build/pkgs/python2/patches/: all the files etc are as they should be, yet I get

[python2-2.7.13.p1] Applying patches from ../patches...
[python2-2.7.13.p1] Applying ../patches/2.6.5-FD_SETSIZE.patch
[python2-2.7.13.p1] patching file Modules/selectmodule.c
[python2-2.7.13.p1] Applying ../patches/21_all_distutils_c++.patch
[python2-2.7.13.p1] can't find file to patch at input line 5
[python2-2.7.13.p1] Perhaps you used the wrong -p or --strip option?
[python2-2.7.13.p1] The text leading up to this was:
[python2-2.7.13.p1] --------------------------
[python2-2.7.13.p1] |https://bugs.python.org/issue1222585
[python2-2.7.13.p1] |
[python2-2.7.13.p1] |--- Lib/_osx_support.py
[python2-2.7.13.p1] |+++ Lib/_osx_support.py
[python2-2.7.13.p1] --------------------------
[python2-2.7.13.p1] File to patch: 
...

Seems to be the right format, the right paths to files, yet this...

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

Check the patch directory levels with another patch there may be one directory to many in the Gentoo patch compared to what sage's automatic patching needs.

comment:18 in reply to: ↑ 17 Changed 3 years ago by dimpase

Replying to fbissey:

Check the patch directory levels with another patch there may be one directory to many in the Gentoo patch compared to what sage's automatic patching needs.

no, it's not the case as far as I can see (and I've been stuck with it for an hour already, just not yet ready to apply it by hand...)

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

Just checked you need to _add_ one directory level to the gentoo patch. That is

--- Lib/_osx_support.py
+++ Lib/_osx_support.py

needs to become

--- a/Lib/_osx_support.py
+++ b/Lib/_osx_support.py

and so on...

comment:20 in reply to: ↑ 19 Changed 3 years ago by dimpase

Replying to fbissey:

Just checked you need to _add_ one directory level to the gentoo patch. That is

--- Lib/_osx_support.py
+++ Lib/_osx_support.py

needs to become

--- a/Lib/_osx_support.py
+++ b/Lib/_osx_support.py

and so on...

Oh, thanks, I've been blind...

comment:21 in reply to: ↑ 3 Changed 3 years ago by dimpase

Replying to fbissey:

OK :)

The next question is how to ensure that the compiler then uses c++11. lcalc doesn't have a configure script so we cannot do it there. At the very least spkg-install will need to insert -std=c++11 in CXXFLAGS to make sure you are always using c++11.

Somehow I cannot manage to achieve this; it all works if I do

CXXFLAGS="-std=c++11" ./sage -f lcalc

but somehow does not work if I put into spkg-install

$MAKE CXXFLAGS="-std=c++11"

some evil Makefile magic that escapes me here. :-(

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

Most other spkg-install do something like

CXXFLAGS="-std=c++11 ${CXXFLAGS}"
export CXXFLAGS
$MAKE

comment:23 in reply to: ↑ 22 Changed 3 years ago by dimpase

Replying to fbissey:

Most other spkg-install do something like

CXXFLAGS="-std=c++11 ${CXXFLAGS}"
export CXXFLAGS
$MAKE

no difference, it still does not work. It might be due to the Makefile modifying CXXFLAGS: it does

CXXFLAGS := -O3 $(OPENMP_FLAG) $(PREPROCESSOR_DEFINE) $(MACHINE_SPECIFIC_FLAGS) $(EXTRA) $(CXXFLAGS)

comment:24 Changed 3 years ago by dimpase

I don't know:

  • bash bug? (version 4.4.12(1))
  • make bug? (4.2.1)

will try on something older...

comment:25 Changed 3 years ago by fbissey

logs please.

comment:26 follow-ups: Changed 3 years ago by rws

  • Component changed from PLEASE CHANGE to number theory
  • Dependencies #12426 deleted
  • Milestone changed from sage-8.0 to sage-8.1
  • Type changed from PLEASE CHANGE to enhancement

I'm willing to do some bulk C++11 work, as I did with Pynac. Is there a public lcalc repo?

comment:27 Changed 3 years ago by dimpase

  • Branch changed from u/dimpase/lcalc_c11 to u/dimpase/lcalc_cxx11
  • Commit changed from 14e08ae6eb3f2d5c85f8b1e307ac5c1160d224ee to 666d6a3d3de0d80ff8730dd3320f182b6f21368b

this is the branch, as here are the results of running

./sage -f lcalc

http://users.ox.ac.uk/~coml0531/sage/lcalc-1.23.p16.log

vs the result of running

CXXFLAGS="-std=c++11" ./sage -f lcalc

http://users.ox.ac.uk/~coml0531/sage/externalCXXFLAGS_lcalc-1.23.p16.log

As you see in the 1st log CXXFLAGS are ignored. Tested on Fedora25 as well as on Gentoo. What am I missing?!!

PS. F25 bash is 4.3.43(1)-release, and make is 4.1.

Last edited 3 years ago by dimpase (previous) (diff)

comment:28 in reply to: ↑ 26 ; follow-up: Changed 3 years ago by dimpase

Replying to rws:

I'm willing to do some bulk C++11 work, as I did with Pynac. Is there a public lcalc repo?

It's done here, the problem which remains is to fight the makefile madness

There will be more similar C++ "fun" for gfan (I have a patch, but it produces lots of test errors), giac... I'll cc you on these tickets as they appear.

comment:29 in reply to: ↑ 26 Changed 3 years ago by fbissey

Replying to rws:

I'm willing to do some bulk C++11 work, as I did with Pynac. Is there a public lcalc repo?

No.

comment:30 Changed 3 years ago by dimpase

let us make one on github.

comment:31 in reply to: ↑ 28 Changed 3 years ago by rws

Replying to dimpase:

There will be more similar C++ "fun" for gfan (I have a patch, but it produces lots of test errors), giac... I'll cc you on these tickets as they appear.

Note BTW there is gfan-0.6 now.

comment:32 follow-up: Changed 3 years ago by dimpase

in fact, a lcalc repo is here : https://code.google.com/archive/p/l-calc/

last commit is from 2014, but it's newer than what we use, see http://oto.math.uwaterloo.ca/~mrubinst/L_function_public/CODE/

comment:33 Changed 3 years ago by rws

I just did one for gfan: https://github.com/rwst/gfan06

comment:34 Changed 3 years ago by fbissey

This is quite peculiar. Looking more closely, even the section for OS X in spkg-install, doesn't work properly

if [ "$UNAME" = "Darwin" ]; then
    export LDFLAGS="$LDFLAGS -Wl,-headerpad_max_install_names"
fi

we still get the right thing because there is darwin section in the makefile but the export above doesn't work.

comment:35 in reply to: ↑ 32 Changed 3 years ago by dimpase

Replying to dimpase:

in fact, a lcalc repo is here : https://code.google.com/archive/p/l-calc/

last commit is from 2014, but it's newer than what we use, see http://oto.math.uwaterloo.ca/~mrubinst/L_function_public/CODE/

I've imported the code.google (hg) repo into git, here: https://github.com/dimpase/lcalc

I'll try merging Sage's changes there.

comment:36 follow-up: Changed 3 years ago by rws

That repo code is different enough and clang-check gives lots of errors. If we do this together, how to coordinate?

comment:37 in reply to: ↑ 36 ; follow-up: Changed 3 years ago by dimpase

Replying to rws:

That repo code is different enough and clang-check gives lots of errors. If we do this together, how to coordinate?

If you like to take the lead on this, please go ahead---I have my hands full with other things.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 3 years ago by rws

Replying to dimpase:

If you like to take the lead on this, please go ahead---I have my hands full with other things.

NP. Actual code changes will go in a PR.

comment:39 in reply to: ↑ 38 Changed 3 years ago by rws

Replying to rws:

NP. Actual code changes will go in a PR.

https://github.com/dimpase/lcalc/pull/1

Please review. I haven't worked on upgrading the Sage package, nor have I tested functionality.

comment:40 Changed 2 years ago by jdemeyer

Two questions about this ticket:

  1. Is it about lcalc itself, Sage modules using lcalc, or both?
  1. Instead of porting lcalc to C++11, is it an option to force -std=c++98 instead?

comment:41 Changed 2 years ago by jdemeyer

  • Description modified (diff)

Hmm, maybe it's not a matter of porting to C++11, but just porting to proper C++ instead. At least src/sage/libs/lcalc/lcalc_Lfunction.pyx doesn't compile with -std=c++98 either: it requires either -std=gnu++98 or -std=gnu++11.

comment:42 in reply to: ↑ 13 Changed 2 years ago by jdemeyer

Replying to fbissey:

I think Dima started this with the view of compiling sage with -std=c++11. If that's what he is doing, getting https://bugs.python.org/issue1222585 in would be more pressing in my opinion.

I created #7028 for that.

comment:43 follow-up: Changed 2 years ago by dimpase

There is one more thing to add here; there is a macro double(x) defined in include/Lcommon.h, which does not seem to be used anywhere, and which causes havoc sometimes, e.g. if I try building with gcc on FreeBSD (due to cephes present)

Anyway, hey, redefining a language keyword, yuck...

comment:44 Changed 2 years ago by rws

Because of what I logged in my comments in #23898 it looks like current lcalc does not compile on OpenSuSE systems with system gcc-5.3.1. A patch is at https://trac.sagemath.org/ticket/23898#comment:39.

comment:45 in reply to: ↑ 43 Changed 2 years ago by rws

Replying to dimpase:

There is one more thing to add here; there is a macro double(x) defined in include/Lcommon.h, which does not seem to be used anywhere, and which causes havoc sometimes, e.g. if I try building with gcc on FreeBSD (due to cephes present)

This is outcommented in the github lcalc version which also, with the above PR, passes clang-check. It looks like an improvement. OTOH this ticket seems no longer pressing, as the Sage version compiles with clang too.

The attempt to replace Sage's lcalc with Pari (#24532) was dropped, so maybe it's an idea to at least upgrade it to the github version?

comment:46 follow-up: Changed 2 years ago by dimpase

I am all for it, but I'm a bit lost here. What stage are we at?

comment:47 in reply to: ↑ 46 Changed 2 years ago by rws

Replying to dimpase:

What stage are we at?

This is the first ticket that mentions the 2014 version at all I think.

comment:48 Changed 2 years ago by rws

There is mention in https://trac.sagemath.org/ticket/11321#comment:8 but this is only because patches were sent upstream.

comment:49 follow-up: Changed 2 years ago by rws

The github version defines itself in CHANGE.LOG and include/cmdline.h as v. 1.3, so we might stick to that and add something. I mean with that the name of the tarball later distributed, not our Sage patchlevel. I personally prefer ISO dates like 20180223 whenever possible.

comment:50 in reply to: ↑ 49 ; follow-up: Changed 2 years ago by jdemeyer

Replying to rws:

I personally prefer ISO dates like 20180223 whenever possible.

What does the date refer to? If you are packaging something which does not have a naturally defined date, I wouldn't use a date. For packages created from a git repo, better use a git commit hash.

comment:51 in reply to: ↑ 50 Changed 2 years ago by rws

Replying to jdemeyer:

For packages created from a git repo, better use a git commit hash.

That's fine too.

comment:52 Changed 2 years ago by rws

The changes to install the package without patches are now added to https://github.com/dimpase/lcalc/pull/1 which eventually needs a review. ATM Sage does NOT build with it as compilation of the Cython module fails.

One problem worked around was that the use of FP type (double,mpfr,etc..) can be given to make but is not fixed in a header like with conf.h in other packages. I have hardcoded the type used until now (double) but this is an issue.

comment:53 follow-up: Changed 2 years ago by dimpase

I've just looked at the pull request again - do you see my comments there?

comment:54 in reply to: ↑ 53 Changed 2 years ago by rws

Replying to dimpase:

I've just looked at the pull request again - do you see my comments there?

Yes, I have answered there.

comment:55 follow-up: Changed 2 years ago by rws

I opened #24280 with the changes on the Sage side. Testing yields mostly tolerance exceeded but also some different values that need a look.

comment:56 in reply to: ↑ 55 Changed 2 years ago by dimpase

Replying to rws:

I opened #24280 with the changes on the Sage side. Testing yields mostly tolerance exceeded but also some different values that need a look.

seems to be wrong reference, what is the correct ticket?

comment:57 Changed 2 years ago by rws

Please see #24820.

comment:58 Changed 2 years ago by chapoton

  • Keywords lcalc added

comment:59 follow-ups: Changed 19 months ago by gh-timokau

The ntl upgrade (#25532) depends on this. Is there a reason this (or #24820) isn't needs_review?

comment:60 in reply to: ↑ 59 Changed 19 months ago by rws

Replying to gh-timokau:

The ntl upgrade (#25532) depends on this. Is there a reason this (or #24820) isn't needs_review?

I think #24820 is the newer attempt. It would be necessary to agree that either the Hardy-Z function is not defined for complex argument or lcalc is unable to compute the case. As I said in #24820, I have no idea about the usefulness of complex argument results. Do you?

comment:61 in reply to: ↑ 59 ; follow-up: Changed 19 months ago by jdemeyer

Replying to gh-timokau:

The ntl upgrade (#25532) depends on this.

As I said on that ticket, the dependency itself could be considered a bug. It is probably easier to remove the dependency than fixing lcalc.

comment:62 in reply to: ↑ 61 Changed 19 months ago by gh-timokau

Replying to rws:

Replying to gh-timokau:

The ntl upgrade (#25532) depends on this. Is there a reason this (or #24820) isn't needs_review?

I think #24820 is the newer attempt. It would be necessary to agree that either the Hardy-Z function is not defined for complex argument or lcalc is unable to compute the case. As I said in #24820, I have no idea about the usefulness of complex argument results. Do you?

Unfortunately I don't :/

Replying to jdemeyer:

Replying to gh-timokau:

The ntl upgrade (#25532) depends on this.

As I said on that ticket, the dependency itself could be considered a bug. It is probably easier to remove the dependency than fixing lcalc.

At least that would fix #25532. Isn't the dependency there for a reason?

comment:63 Changed 15 months ago by fbissey

I think it is time we move forward with this again. I think this ticket should also include the matching patch from arch in sagelib https://git.archlinux.org/svntogit/community.git/plain/trunk/sagemath-lcalc-c++11.patch?h=packages/sagemath&id=0e31ae526ab7c6b5c0bfacb3f8b1c4fd490035aa so that we really are just using C++11 and allow us to make progress on #25532.

Any objections to moving forward with what we have now?

comment:64 Changed 15 months ago by fbissey

Yes this need the extra patch from arch I quoted. Otherwise sage's compilation fails because lcalc is no using construct that don't exist in gnu++98.

comment:65 follow-up: Changed 15 months ago by fbissey

  • Authors set to Dima Pasechnik, François Bissey
  • Branch changed from u/dimpase/lcalc_cxx11 to u/fbissey/lcalc_cxx11
  • Commit changed from 666d6a3d3de0d80ff8730dd3320f182b6f21368b to 837732e21257e3d2ad14bff15424ea9cae197e1b
  • Milestone changed from sage-8.1 to sage-8.5
  • Status changed from new to needs_review

Last call for patches. Please someone review. It would be good to have this in the next beta and be able to move on to #25532 and get it all done for 8.5.


New commits:

108d581Merge branch 'develop' into lcalc_cxx11
834e7a6lcalc version bump for rebuild
837732eremove enforcement of gnu++98 in lcalc bindings

comment:66 Changed 15 months ago by fbissey

Anyone interested in reviewing this?

comment:67 Changed 15 months ago by jdemeyer

  1. Can you add some rationale for the changes to Makefile?
  1. I wouldn't make style changes such as
    -               loop(i,this->N,N)
    -                       coeffs[i]=zero;
    +               loop(i,this->N,N) coeffs[i]=zero;
    

comment:68 in reply to: ↑ 65 ; follow-up: Changed 15 months ago by jdemeyer

Replying to fbissey:

It would be good to have this in the next beta and be able to move on to #25532

As I mentioned multiple times, I don't really think that #25532 must depend on this.

comment:69 Changed 15 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:70 in reply to: ↑ 68 Changed 15 months ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

It would be good to have this in the next beta and be able to move on to #25532

As I mentioned multiple times, I don't really think that #25532 must depend on this.

Well, I tried to see if I could up ntl separately in sage-on-gentoo and sage wouldn't compile. It could be that I can tweak ntl's configuration to be compatible (at possible cost in ntl internal) but I'd rather get rid of the last bit of sage that is c++11 resistant (I think) and go with the natural flow again.

comment:71 follow-up: Changed 15 months ago by jdemeyer

Minor nitpick: wouldn't it be cleaner to replace decltype(m) i=(m) by auto i = (m)?

Last edited 15 months ago by jdemeyer (previous) (diff)

comment:72 in reply to: ↑ 71 ; follow-up: Changed 15 months ago by fbissey

Replying to jdemeyer:

Minor nitpick: wouldn't it be cleaner to replace decltype(m) i=(m) by auto i = (m)?

It has indeed a better feel. A bit too late in the day for me to do any changes (safely), I'll go through your reviews in the morning.

comment:73 in reply to: ↑ 72 ; follow-up: Changed 15 months ago by jdemeyer

Replying to fbissey:

A bit too late in the day for me

You chose to live on the wrong side of the world :-)

comment:74 in reply to: ↑ 73 Changed 15 months ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

A bit too late in the day for me

You chose to live on the wrong side of the world :-)

Well it feels like it will be a nice summer. And not too hot, like our australian neighbors :-) but I really need to get some sleep now.

comment:75 Changed 15 months ago by jdemeyer

In your Makefile patch, the rule $(CC) $(CFLAGS) $(INCLUDEFILES) -c cmdline.c is just the standard build rule for .c files (modulo the typo $(CFLAGS)->$(CCFLAGS)). So it might be better to just restore the comment.

Last edited 15 months ago by jdemeyer (previous) (diff)

comment:76 follow-up: Changed 15 months ago by fbissey

OK some comments on the review from Jeroen which made me dig a bit deeper on the Makefile. I must say I adopted a slightly different approach in sage-on-gentoo so I don't have the same patch, but some of lesson learnt will definitely apply.

  • the original makefile defines CC as as g++ so that has to go.
  • Non standard flags like CCFLAGS are used, so Dima tried to use appropriate flags for both C and C++ compilers. And it also differentiate C and C++ flags which wasn't done before because even regular C code was passed to the C++ compiler.
  • The downside of using CFLAGS and CXXFLAGS inside the makefile is that regular options passed from the sage build system may need care if we want to inject them. That's a sage-on-gentoo difference I use MYCXXFLAGS so I can include whatever other flags we pass without worry.
  • Jeroen points out the standard rule for .c file should be used for cmdline.c except that the current patch erroneously define the rule as using CXXFLAGS instead of CFLAGS. Oopsie.

Point of difference with sage-on-gentoo: because pari headers are included in two files with #include "pari.h" the makefile has to define the location of pari headers and so do any code that include these headers. Decided that it was not worth it and patched to have #include "pari/pari.h" which is more sane and means that we don't have to worry about adding non standard includes. That means that I cut an extra bit of the makefile too. If people think that's a good idea I'll include that change in sage too.

comment:77 in reply to: ↑ 76 Changed 15 months ago by jdemeyer

Replying to fbissey:

because pari headers are included in two files with #include "pari.h" the makefile has to define the location of pari headers and so do any code that include these headers. Decided that it was not worth it and patched to have #include "pari/pari.h" which is more sane and means that we don't have to worry about adding non standard includes. That means that I cut an extra bit of the makefile too. If people think that's a good idea I'll include that change in sage too.

That makes sense to me.

comment:78 Changed 15 months ago by git

  • Commit changed from 837732e21257e3d2ad14bff15424ea9cae197e1b to eb072d1119cd2305bb02cd8f8e00168e67f5cad6

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

8190cefCorrect inclusion of pari header so no extra -I flags are required.
71f8bf2update makefile patch and document it
eb072d1update the patch according to Jeroen's review. Add a correction of a "std:" which should have been "std::"

comment:79 Changed 15 months ago by fbissey

  • Status changed from needs_work to needs_review

All right back to review.

comment:80 Changed 15 months ago by git

  • Commit changed from eb072d1119cd2305bb02cd8f8e00168e67f5cad6 to 3fb7f80a02461137b5e1e80cfd5fae6ba9784b6a

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

3fb7f80std: was introduced in the clang patch. Correcting in that patch.

comment:81 Changed 15 months ago by fbissey

So many patches! It is hard to figure if the thing you patch was introduced by another patch :P

comment:82 follow-up: Changed 15 months ago by dimpase

This appears to work with clang 6.0.1, a good sign.

comment:83 in reply to: ↑ 82 Changed 14 months ago by fbissey

Replying to dimpase:

This appears to work with clang 6.0.1, a good sign.

I have checked clang-6 and gcc-8.2. Now if someone would put a kind review.

Although Dima, I kept the patch name "using_namespace_std.patch` but there is hardly any of that in there. Just removing GNU-ism. Did you have more stuff to do in mind?

comment:84 Changed 14 months ago by dimpase

  • Reviewers set to Dima Pasechnik, François Bissey, Jeroen Demeyer, Ralf Stephan
  • Status changed from needs_review to positive_review

OK

comment:85 Changed 14 months ago by gh-timokau

Thank you @fbissey for pushing this forward, happy to have one less patch to apply :)

comment:86 Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work

On some machines this fails with:

make libLfunction.so
make[5]: Entering directory '/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/lcalc-1.23.p17/src/src'
g++ -O3   -ffast-math -fPIC   -I../include -c Lglobals.cc
In file included from ../include/Lglobals.h:48:0,
                 from Lglobals.cc:23:
../include/Lcommon.h: In member function 'void smallPoly<T>::resize(int)':
../include/Lcommon.h:71:8: error: 'i' does not name a type
   loop(i,this->N,N)
        ^
../include/Lcommon.h:51:30: note: in definition of macro 'loop'
 #define loop(i,m,n) for(auto i=(m); i!=(n); i++)
                              ^

I guess the -std=c11 is missing in the gcc invocation...

comment:87 Changed 14 months ago by fbissey

Of course I tested with compilers that default to C++11 or C++14 so that would make sense. Will fix ASAP.

Last edited 14 months ago by fbissey (previous) (diff)

comment:88 Changed 14 months ago by fbissey

lcalc has both spkg-build and spkg-install, the -std=c++11 was only added to spkg-install which explains the result. I am going to further inspect those for improvement.

comment:89 Changed 14 months ago by git

  • Commit changed from 3fb7f80a02461137b5e1e80cfd5fae6ba9784b6a to 7cf967fa5f50daf67e9a910786970bf184c37ecb

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

7cf967fmake sure -std=c++11 is passed during build. Various improvement to spkg-build and spkg-install.

comment:90 Changed 14 months ago by fbissey

  • Status changed from needs_work to needs_review

Back to need review.

comment:91 follow-up: Changed 14 months ago by dimpase

  • Status changed from needs_review to positive_review

over to the bots!

comment:92 in reply to: ↑ 91 Changed 14 months ago by fbissey

Replying to dimpase:

over to the bots!

Thank you Dima. Once we have closure here, I would appreciate if you could push the review button on #25532 for the ntl upgrade.

comment:93 Changed 14 months ago by vbraun

  • Branch changed from u/fbissey/lcalc_cxx11 to 7cf967fa5f50daf67e9a910786970bf184c37ecb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.