fixing c++ issues in eclib #22711

dimpase · 2017-03-29T22:25:58Z

clang++ refuses to guess the correct return statement. Also, delete[] must not be mixed up with delete. This ticket fixes these issues, and allows #22679 to go forward.

The tarball at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20170330.tar.bz2

Upstream: None of the above - read trac for reasoning.

CC: @kiwifb @JohnCremona @jdemeyer

Component: packages: standard

Author: Dima Pasechnik

Branch/Commit: 4d2f8db

Reviewer: Jeroen Demeyer

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

The text was updated successfully, but these errors were encountered:

dimpase · 2017-03-29T22:26:28Z

Changed branch from u/dimpase/cxx_eclib to u/dimpase/cxx_ecl

dimpase · 2017-03-29T22:26:28Z

Commit: 4c4cca8

dimpase · 2017-03-29T22:26:28Z

New commits:

`4c4cca8`	`more C++ standard conformance`

dimpase · 2017-03-29T22:46:21Z

comment:2

Please have a look. This is a very straightfoward change, basically just following meaningful warnings emitted by clang++.

dimpase · 2017-03-29T22:46:21Z

Upstream: Reported upstream. No feedback yet.

dimpase · 2017-03-29T22:54:29Z

comment:3

John, it seems that a large part of it is already in your https://github.com/JohnCremona/eclib

Perhaps you can just pick what's not yet there from the patch, and make a new release...

JohnCremona · 2017-03-30T07:51:15Z

comment:4

OK perhaps, but not until next week. I am very receptive to pull requests into eclib. I am strongly opposed to patching eclib for Sage independently of that.

Note also that there has been a new release of eclib for several weeks waiting for a positive review --jdemeyer got close but then it got forgotten.

Please change the "no feedback yet" tag -- this is feedback.

kiwifb · 2017-03-30T07:56:05Z

comment:5

eclib-20170122 is in Volker's tree. I believe it is positively reviewed and should be in the next beta. Do you mean a more recent one? I cannot see a more recent tag on github.

jdemeyer · 2017-03-30T07:59:37Z

comment:6

Replying to @kiwifb:

eclib-20170122 is in Volker's tree. I believe it is positively reviewed and should be in the next beta.

I agree: #22164 has been closed. Just wait for the next beta.

JohnCremona · 2017-03-30T08:04:42Z

comment:7

That's good, I missed that had happened 3 days ago.

dimpase · 2017-03-30T10:39:48Z

comment:8

OK, let me see if this patch adds anything to what's already done on #22164.

dimpase · 2017-03-30T11:29:35Z

comment:9

The patch here catches one more missing [] in delete (thus a memory leak) in libsrc/newforms.cc (after line 1665), a missing return bd; just after line 109 in libsrc/htconst.cc (something which clang++ 3.8 does not forgive you, resulting in lots of errors).

So these absolutely must be fixed.

There is also some more, regarding shifting negative values (undefined behavour in C++).

For the latter, I haven't done the complete job---it appears to be still working without it. Let's complete this fixing, cause it's just trouble in waiting to hit after a compiler update...
One should start off #22164, naturally.

dimpase · 2017-03-30T11:29:35Z

Dependencies: #22164

JohnCremona · 2017-03-30T11:47:07Z

comment:10

Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?

I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.

Can you give one example of where the code shifts negative values?

JohnCremona · 2017-03-30T11:58:52Z

comment:11

I have a version 20170330 with the two fixes in and an increment to the library version.

I can push that now to https://github.com/JohnCremona/eclib or add some more -- I have time to make these edits but not to go searching for things a compiler I don't have picks up.

dimpase · 2017-03-30T12:02:13Z

comment:12

Replying to @JohnCremona:

Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?

Will do. For the master branch, right?

I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.

Can you give one example of where the code shifts negative values?

in the diff fragment below #define QS_LONG_MASK (~(-1L<<QS_LONG_SHIFT))
is such an example; putting extra pair of brackets as done in the diff fixes this.

--- a/libsrc/eclib/sieve_search.h
+++ b/libsrc/eclib/sieve_search.h
@@ -76,7 +76,7 @@ class point_counter : public point_processor {
 #define QS_LONG_SHIFT ((QS_LONG_LENGTH == 16) ? 4 : \
                     (QS_LONG_LENGTH == 32) ? 5 : \
 		    (QS_LONG_LENGTH == 64) ? 6 : 0)
-#define QS_LONG_MASK (~(-1L<<QS_LONG_SHIFT))
+#define QS_LONG_MASK (~(-(1L<<QS_LONG_SHIFT)))
 #define QS_HALF_MASK ((bit_array)(~((unsigned long)(-1L) / 3)))
 
      //#define PERCENT 0.6

This matches one from ratpoints I fixed some time ago. But there are more, hopefully equally unambiguous.

It's more problematic if you do x<<y for x a signed variable;
I'd probably not want to touch these myself, as this would require either changing the type of x, or copying abs(x) to an unsigned thing, and putting the sign back then.

JohnCremona · 2017-03-30T12:08:52Z

comment:13

Replying to @dimpase:

Replying to @JohnCremona:

Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?

Will do. For the master branch, right?

Hold off since I have already fixed the two specific things you mentioned.

I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.

Can you give one example of where the code shifts negative values?

in the diff fragment below #define QS_LONG_MASK (~(-1L<<QS_LONG_SHIFT))
is such an example; putting extra pair of brackets as done in the diff fixes this.
--- a/libsrc/eclib/sieve_search.h
+++ b/libsrc/eclib/sieve_search.h
@@ -76,7 +76,7 @@ class point_counter : public point_processor {
 #define QS_LONG_SHIFT ((QS_LONG_LENGTH == 16) ? 4 : \
                     (QS_LONG_LENGTH == 32) ? 5 : \
 		    (QS_LONG_LENGTH == 64) ? 6 : 0)
-#define QS_LONG_MASK (~(-1L<<QS_LONG_SHIFT))
+#define QS_LONG_MASK (~(-(1L<<QS_LONG_SHIFT)))
 #define QS_HALF_MASK ((bit_array)(~((unsigned long)(-1L) / 3)))
 
      //#define PERCENT 0.6
This matches one from ratpoints I fixed some time ago. But there are more, hopefully equally unambiguous.

That's unfortunate -- this code was adapted from a (now very old) version of ratpoints, and that sort of macro is not my style at all.

It's more problematic if you do x<<y for x a signed variable;
I'd probably not want to touch these myself, as this would require either changing the type of x, or copying abs(x) to an unsigned thing, and putting the sign back then.

I am not sure what to do next, so I will push the changes I already made to github.

JohnCremona · 2017-03-30T12:17:31Z

comment:14

I also have a new eclib-20170330.tar.bz2 ready to post if we make no more code changes, or I can remake it if we do.

dimpase · 2017-03-30T12:21:22Z

comment:15

the rapoints fix was done in the branch on #12473, including the macro fix I propose in the diff in the comment 12 on this (#22711) ticket, so I don't see why you won't include this in the tarball, too.

JohnCremona · 2017-03-30T12:29:16Z

comment:16

Replying to @dimpase:

the rapoints fix was done in the branch on #12473, including the macro fix I propose in the diff in the comment 12 on this (#22711) ticket, so I don't see why you won't include this in the tarball, too.

OK I will do that. Without a further increase in version number since I have not distributed that tarball yet.

JohnCremona · 2017-03-30T12:36:24Z

comment:17

Done. The extra fix is now in the master at github and the new distribution file is at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20170330.tar.bz2

dimpase · 2017-03-30T12:41:33Z

comment:20

in case, I can't access the new tarball:

Forbidden
You don't have permission to access /staff/J.E.Cremona/ftp/eclib-20170330.tar.bz2 on this server.
Apache/1.3.27 Server at homepages.warwick.ac.uk Port 80

JohnCremona · 2017-03-30T12:47:18Z

comment:21

Replying to @dimpase:

in case, I can't access the new tarball:

Forbidden
You don't have permission to access /staff/J.E.Cremona/ftp/eclib-20170330.tar.bz2 on this server.
Apache/1.3.27 Server at homepages.warwick.ac.uk Port 80

Sorry, fixed now.

sagetrac-git · 2017-03-30T13:35:58Z

Changed commit from 4c4cca8 to 9dd970f

sagetrac-git · 2017-03-30T13:35:58Z

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

`9dd970f`	`use the latest upstream tarball`

dimpase · 2017-03-30T13:38:11Z

Changed upstream from Reported upstream. No feedback yet. to None of the above - read trac for reasoning.

jdemeyer · 2017-03-30T20:04:28Z

comment:25

This branch should be based on #22164.

jdemeyer · 2017-03-30T20:04:28Z

Reviewer: Jeroen Demeyer

JohnCremona · 2017-03-31T07:43:33Z

comment:26

... and #22164 is now in 8.0.beta0.

jdemeyer · 2017-03-31T08:38:05Z

Changed dependencies from #22164 to none

jdemeyer · 2017-03-31T08:38:05Z

comment:27

Replying to @JohnCremona:

... and #22164 is now in 8.0.beta0.

Right. So it needs to be rebased on that.

JohnCremona · 2017-03-31T08:45:35Z

comment:28

But note that the original patch / branch posted here by dimpase is no longer needed since it consisted of patches to eclib which have been fixed upstream; so all that is needed here is a trivial new branch to refer to the new eclib version. I will do that now (where "now" = "as soon as I have built 8.0.beta0").

sagetrac-git · 2017-03-31T10:09:23Z

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

`4d2f8db`	`rebased over the new \beta`

sagetrac-git · 2017-03-31T10:09:23Z

Changed commit from 9dd970f to 4d2f8db

dimpase · 2017-03-31T10:10:26Z

comment:30

Here cometh the rebased branch.

JohnCremona · 2017-03-31T10:41:16Z

comment:31

Fantastic. I will test your branch on my newly built 8.0beta0 and report back. I hope it will not be seen as a conflict of interest for me to review this!

dimpase · 2017-03-31T12:12:34Z

comment:32

Replying to @JohnCremona:

I hope it will not be seen as a conflict of interest for me to review this!

You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...

JohnCremona · 2017-03-31T12:15:33Z

comment:33

Replying to @dimpase:

Replying to @JohnCremona:

I hope it will not be seen as a conflict of interest for me to review this!

You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...

Now which country did you grow up in, I seem to have forgotten?

Running ptestlong gives only these:

sage -t --long --warn-long 111.3 src/sage/homology/simplicial_complex.py  # 1 doctest failed
sage -t --long --warn-long 111.3 src/sage/calculus/interpolators.pyx  # Timed out (and interrupt failed)
sage -t --long --warn-long 111.3 src/sage/calculus/riemann.pyx  # Timed out (and interrupt failed)
sage -t --long --warn-long 111.3 src/sage/matrix/matrix_double_dense.pyx  # Timed out (and interrupt failed)
sage -t --long --warn-long 111.3 src/sage/finance/time_series.pyx  # Timed out (and interrupt failed)

none of w3hich are related to eclib -- and it's a beta release which I had not tested with the old eclib.

dimpase · 2017-03-31T12:23:37Z

comment:34

Replying to @JohnCremona:

Replying to @dimpase:

Replying to @JohnCremona:

I hope it will not be seen as a conflict of interest for me to review this!

You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...

Now which country did you grow up in, I seem to have forgotten?

Let me assure you that I grew up listening to BBC :-)

vbraun · 2017-04-05T19:38:07Z

Changed branch from u/dimpase/cxx_ecl to 4d2f8db

dimpase added this to the sage-8.0 milestone Mar 29, 2017

dimpase added c: packages: standard labels Mar 29, 2017

dimpase added p: critical / 2 and removed p: major / 3 labels Mar 30, 2017

This comment has been minimized.

Sign in to view

dimpase added the s: needs review label Mar 30, 2017

jdemeyer added s: needs work and removed s: needs review labels Mar 30, 2017

dimpase added s: needs review and removed s: needs work labels Mar 31, 2017

jdemeyer added s: positive review and removed s: needs review labels Apr 2, 2017

vbraun removed the s: positive review label Apr 5, 2017

vbraun closed this as completed in 5fb056a Apr 5, 2017

fixing c++ issues in eclib #22711

fixing c++ issues in eclib #22711

Comments

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

dimpase commented Mar 29, 2017

JohnCremona commented Mar 30, 2017

kiwifb commented Mar 30, 2017

jdemeyer commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

dimpase commented Mar 30, 2017

dimpase commented Mar 30, 2017

dimpase commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

dimpase commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

dimpase commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

This comment has been minimized.

This comment has been minimized.

dimpase commented Mar 30, 2017

JohnCremona commented Mar 30, 2017

sagetrac-git mannequin commented Mar 30, 2017

sagetrac-git mannequin commented Mar 30, 2017

This comment has been minimized.

dimpase commented Mar 30, 2017

jdemeyer commented Mar 30, 2017

jdemeyer commented Mar 30, 2017

JohnCremona commented Mar 31, 2017

jdemeyer commented Mar 31, 2017

jdemeyer commented Mar 31, 2017

JohnCremona commented Mar 31, 2017

sagetrac-git mannequin commented Mar 31, 2017

sagetrac-git mannequin commented Mar 31, 2017

dimpase commented Mar 31, 2017

JohnCremona commented Mar 31, 2017

dimpase commented Mar 31, 2017

JohnCremona commented Mar 31, 2017

dimpase commented Mar 31, 2017

vbraun commented Apr 5, 2017