Opened 5 years ago
Closed 3 years 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, GitHub, GitLab) | Commit: | 7cf967fa5f50daf67e9a910786970bf184c37ecb |
Dependencies: | Stopgaps: |
Description (last modified by )
lcalc doesn't use standard C++:
using namespace std;
needs an include with stuff fromstd::
before it
typeof
is a gcc extension - C++11 hasdecltype
instead
- plain C must not be compiled with C++ compiler - thus a makefile change
Change History (93)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
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: ↓ 6 ↓ 21 Changed 5 years ago by
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 5 years ago by
- Commit changed from 015955efe9577dc9c3d443a4328297cd097454ee to 14e08ae6eb3f2d5c85f8b1e307ac5c1160d224ee
Branch pushed to git repo; I updated commit sha1. New commits:
5434c70 | Merge branch 'develop' into compiler
|
f565739 | bump config tarball
|
53f9c4d | Merge branch 'compiler' into clang-progress
|
7a9ab6b | checksum for this branch's configure tarball
|
14e08ae | Merge branch 'u/fbissey/clang-progress' of trac.sagemath.org:sage into clang
|
comment:5 follow-up: ↓ 8 Changed 5 years ago by
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: ↓ 9 Changed 5 years ago by
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
inCXXFLAGS
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 5 years ago by
- 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: ↓ 10 Changed 5 years ago by
- 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 5 years ago by
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 5 years ago by
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 5 years ago by
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: ↓ 13 Changed 5 years ago by
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: ↓ 14 ↓ 42 Changed 5 years ago by
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: ↓ 15 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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: ↓ 18 Changed 5 years ago by
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 5 years ago by
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: ↓ 20 Changed 5 years ago by
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 5 years ago by
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.pyneeds to become
--- a/Lib/_osx_support.py +++ b/Lib/_osx_support.pyand so on...
Oh, thanks, I've been blind...
comment:21 in reply to: ↑ 3 Changed 5 years ago by
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
inCXXFLAGS
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: ↓ 23 Changed 5 years ago by
Most other spkg-install
do something like
CXXFLAGS="-std=c++11 ${CXXFLAGS}" export CXXFLAGS $MAKE
comment:23 in reply to: ↑ 22 Changed 5 years ago by
Replying to fbissey:
Most other
spkg-install
do something likeCXXFLAGS="-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 5 years ago by
I don't know:
- bash bug? (version 4.4.12(1))
- make bug? (4.2.1)
will try on something older...
comment:25 Changed 5 years ago by
logs please.
comment:26 follow-ups: ↓ 28 ↓ 29 Changed 5 years ago by
- 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 5 years ago by
- 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.
comment:28 in reply to: ↑ 26 ; follow-up: ↓ 31 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
let us make one on github.
comment:31 in reply to: ↑ 28 Changed 5 years ago by
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: ↓ 35 Changed 5 years ago by
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 5 years ago by
I just did one for gfan: https://github.com/rwst/gfan06
comment:34 Changed 5 years ago by
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 5 years ago by
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: ↓ 37 Changed 5 years ago by
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: ↓ 38 Changed 5 years ago by
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: ↓ 39 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
Two questions about this ticket:
- Is it about lcalc itself, Sage modules using lcalc, or both?
- Instead of porting lcalc to C++11, is it an option to force
-std=c++98
instead?
comment:41 Changed 5 years ago by
- 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 5 years ago by
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: ↓ 45 Changed 5 years ago by
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 5 years ago by
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 4 years ago by
Replying to dimpase:
There is one more thing to add here; there is a macro
double(x)
defined ininclude/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: ↓ 47 Changed 4 years ago by
I am all for it, but I'm a bit lost here. What stage are we at?
comment:47 in reply to: ↑ 46 Changed 4 years ago by
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 4 years ago by
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: ↓ 50 Changed 4 years ago by
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: ↓ 51 Changed 4 years ago by
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 4 years ago by
Replying to jdemeyer:
For packages created from a git repo, better use a git commit hash.
That's fine too.
comment:52 Changed 4 years ago by
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: ↓ 54 Changed 4 years ago by
I've just looked at the pull request again - do you see my comments there?
comment:54 in reply to: ↑ 53 Changed 4 years ago by
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: ↓ 56 Changed 4 years ago by
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 4 years ago by
comment:57 Changed 4 years ago by
Please see #24820.
comment:58 Changed 4 years ago by
- Keywords lcalc added
comment:59 follow-ups: ↓ 60 ↓ 61 Changed 4 years ago by
comment:60 in reply to: ↑ 59 Changed 4 years ago by
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: ↓ 62 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
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: ↓ 68 Changed 4 years ago by
- 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:
108d581 | Merge branch 'develop' into lcalc_cxx11
|
834e7a6 | lcalc version bump for rebuild
|
837732e | remove enforcement of gnu++98 in lcalc bindings
|
comment:66 Changed 4 years ago by
Anyone interested in reviewing this?
comment:67 Changed 4 years ago by
- Can you add some rationale for the changes to
Makefile
?
- 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: ↓ 70 Changed 4 years ago by
comment:69 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:70 in reply to: ↑ 68 Changed 4 years ago by
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: ↓ 72 Changed 4 years ago by
Minor nitpick: wouldn't it be cleaner to replace decltype(m) i=(m)
by auto i = (m)
?
comment:72 in reply to: ↑ 71 ; follow-up: ↓ 73 Changed 4 years ago by
Replying to jdemeyer:
Minor nitpick: wouldn't it be cleaner to replace
decltype(m) i=(m)
byauto 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: ↓ 74 Changed 4 years ago by
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 4 years ago by
comment:75 Changed 4 years ago by
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.
comment:76 follow-up: ↓ 77 Changed 4 years ago by
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 forcmdline.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 4 years ago by
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 4 years ago by
- Commit changed from 837732e21257e3d2ad14bff15424ea9cae197e1b to eb072d1119cd2305bb02cd8f8e00168e67f5cad6
comment:79 Changed 4 years ago by
- Status changed from needs_work to needs_review
All right back to review.
comment:80 Changed 4 years ago by
- Commit changed from eb072d1119cd2305bb02cd8f8e00168e67f5cad6 to 3fb7f80a02461137b5e1e80cfd5fae6ba9784b6a
Branch pushed to git repo; I updated commit sha1. New commits:
3fb7f80 | std: was introduced in the clang patch. Correcting in that patch.
|
comment:81 Changed 4 years ago by
So many patches! It is hard to figure if the thing you patch was introduced by another patch :P
comment:82 follow-up: ↓ 83 Changed 4 years ago by
This appears to work with clang 6.0.1, a good sign.
comment:83 in reply to: ↑ 82 Changed 3 years ago by
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 3 years ago by
- Reviewers set to Dima Pasechnik, François Bissey, Jeroen Demeyer, Ralf Stephan
- Status changed from needs_review to positive_review
OK
comment:85 Changed 3 years ago by
Thank you @fbissey for pushing this forward, happy to have one less patch to apply :)
comment:86 Changed 3 years ago by
- 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 3 years ago by
Of course I tested with compilers that default to C++11 or C++14 so that would make sense. Will fix ASAP.
comment:88 Changed 3 years ago by
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 3 years ago by
- Commit changed from 3fb7f80a02461137b5e1e80cfd5fae6ba9784b6a to 7cf967fa5f50daf67e9a910786970bf184c37ecb
Branch pushed to git repo; I updated commit sha1. New commits:
7cf967f | make sure -std=c++11 is passed during build. Various improvement to spkg-build and spkg-install.
|
comment:90 Changed 3 years ago by
- Status changed from needs_work to needs_review
Back to need review.
comment:91 follow-up: ↓ 92 Changed 3 years ago by
- Status changed from needs_review to positive_review
over to the bots!
comment:92 in reply to: ↑ 91 Changed 3 years ago by
comment:93 Changed 3 years ago by
- Branch changed from u/fbissey/lcalc_cxx11 to 7cf967fa5f50daf67e9a910786970bf184c37ecb
- Resolution set to fixed
- Status changed from positive_review to closed
Can you rebase on 8.0.rc0? Your branch shows up in red.