Remove nested functions in ratpoints

[ohanar]

these won't work for most other compilers besides GCC.

Upstream: Reported upstream. No feedback yet.

CC: @kiwifb

Component: packages: standard

Author: Dima Pasechnik

Branch: dae689c

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/12473

[dimpase]

comment:5

here is a fix : http://users.ox.ac.uk/~coml0531/sage/ratpoints-2.1.3.tar.bz2
(I moved the only nested function outside; it works with gcc, and passes the tests (in sage)). Wonder how it goes with clang...

Still needs a bit of cleaning.

[kiwifb]

comment:6

Seems to works. I'll work on turning into a patch later.

[dimpase]

Branch: u/dimpase/ratpts_nonestedfun

[dimpase]

comment:7

here is a cleaned up patch, with more const qualifiers added.

---

New commits:

4889e9c

un-nested and added const qualifiers to as many params as possible

[dimpase]

Author: Dima Pasechnik

[dimpase]

Commit: 4889e9c

[kiwifb]

comment:8

Just touching spkg-install since -fnested-functions is not needed anywhere anymore.

---

New commits:

32c21d9

Removing fnested-functions from cflags no that the nested functions are removed.

[kiwifb]

Changed commit from 4889e9c to 32c21d9

[kiwifb]

Changed branch from u/dimpase/ratpts_nonestedfun to u/fbissey/ratpts_nonestedfun

[dimpase]

Reviewer: François Bissey

[dimpase]

comment:11

Thanks!

[kiwifb]

comment:12

I meant to do that ;)

[jdemeyer]

comment:15

Actually, I have the feeling something is missing on this branch, since it only changes CCFLAGS but nothing else.

[dimpase]

comment:16

Replying to @jdemeyer:

Actually, I have the feeling something is missing on this branch, since it only changes CCFLAGS but nothing else.

Francois, the ball is in your court now... (you didn't ask for a review too :))

[kiwifb]

comment:17

Yes indeed, it looks like I messed up when importing Dima's branch.

As for the version bump. Yes I know we could do that. But really we didn't change any functionality, we just made it possible to compile with other compilers. But OK the patch is not trivial.

[sagetrac-git]

Changed commit from 32c21d9 to dae689c

[sagetrac-git]

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

4889e9c	un-nested and added const qualifiers to as many params as possible
15e20ff	Merge branch 'u/dimpase/ratpts_nonestedfun' of trac.sagemath.org:sage into ratpts_nonestedfun
dae689c	Version bump to force rebuild

[kiwifb]

comment:19

Should all be better now.

[dimpase]

comment:20

OK, looks good.

[jdemeyer]

comment:21

Replying to @kiwifb:

As for the version bump. Yes I know we could do that. But really we didn't change any functionality, we just made it possible to compile with other compilers. But OK the patch is not trivial.

The reason I asked for this is mainly to increase testing coverage. If you bump the version, then everybody who upgrades Sage will rebuild the package with the new patch.

[vbraun]

Changed branch from u/fbissey/ratpts_nonestedfun to dae689c

[dimpase]

Upstream: Reported upstream. No feedback yet.

[dimpase]

Changed commit from dae689c to none

[jdemeyer]

comment:24

Just commenting that the functionality that ratpoints provides should be replaced with PARI. It seems that PARI is slowly replacing all number-theoretical packages inside Sage. This already happened with genus2red and it should be possible for lcalc and ratpoints too.

[dimpase]

comment:25

replacing sounds a bit drastic; adding another backend?

[kiwifb]

comment:26

Replying to @dimpase:

replacing sounds a bit drastic; adding another backend?

Honestly, it doesn't. Reducing your number of dependencies because another one, better maintained, can provide the same functionality is not controversial in my opinion. Who doesn't like less maintenance.

[dimpase]

comment:27

ratpoints implements a very tricky mathematics, not something anybody can write. Having more than one implementation is a blessing.

[jdemeyer]

comment:28

I opened #24531 for that, by the way.

[jdemeyer]

comment:29

Replying to @dimpase:

ratpoints implements a very tricky mathematics, not something anybody can write.

The PARI developers are not just "anybody" :-) I trust that they know number theory. Besides, the way that I understand things, the code in PARI is actually based on this ratpoints package. They didn't write it from scratch.

[jdemeyer]

comment:30

Replying to @kiwifb:

Replying to @dimpase:

replacing sounds a bit drastic; adding another backend?

Honestly, it doesn't. Reducing your number of dependencies because another one, better maintained

"better" maintained? I don't think that ratpoints is maintained at all given that the latest release is from 2011.