Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lcalc fails to build with gcc-4.6 #10892

Closed
vbraun opened this issue Mar 8, 2011 · 15 comments
Closed

lcalc fails to build with gcc-4.6 #10892

vbraun opened this issue Mar 8, 2011 · 15 comments

Comments

@vbraun
Copy link
Member

vbraun commented Mar 8, 2011

lcalc fails to build with the gcc-4.6 version in Fedora 15 alpha:

In file included from ../include/Lglobals.h:51:0,
                 from Lglobals.cc:23:
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/limits: In static member function ‘static
 double std::numeric_limits<double>::min()’:
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/limits:1479:83: error: call of overloaded ‘lcalc_to_double(long double)’ is ambiguous
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/limits:1479:83: note: candidates are:
../include/Lcommon.h:18:15: note: double lcalc_to_double(const Double&)
../include/Lcommon.h:29:15: note: double lcalc_to_double(const int&)
../include/Lcommon.h:30:15: note: double lcalc_to_double(const long long int&)
../include/Lcommon.h:31:15: note: double lcalc_to_double(const short int&)
../include/Lcommon.h:32:15: note: double lcalc_to_double(const char&)
../include/Lcommon.h:33:15: note: double lcalc_to_double(const long int&)
../include/Lcommon.h:34:15: note: double lcalc_to_double(const unsigned int&)
../include/Lcommon.h:35:15: note: double lcalc_to_double(const long unsigned int&)

The reason is the following code horror from src/src/include/Lcommon.h (some editing for clarity):

inline double lcalc_to_double(const Double& x) { return x; }
inline double lcalc_to_double(const double& x) { return x; }
//inline double lcalc_to_double(const long double& x) { return x; }
inline double lcalc_to_double(const int& x) { return x; }
inline double lcalc_to_double(const long long& x) { return x; }
inline double lcalc_to_double(const short& x) { return x; }
inline double lcalc_to_double(const char& x) { return x; }
inline double lcalc_to_double(const long int& x) { return x; }
inline double lcalc_to_double(const unsigned int& x) { return x; }
inline double lcalc_to_double(const long unsigned int& x) { return x; }
#define Int(x) (int)(lcalc_to_double(x))
#define Long(x) (Long)(lcalc_to_double(x))
#define double(x) (double)(lcalc_to_double(x))

The last three lines are clearly a bad idea to define before including system headers! As a bandaid, I uncommented the inline double lcalc_to_double(const long double& x), and it compiles fine now. But somebody who is familiar with the codebase should really rewrite lcalc to not redefine the double() cast, thats just fragile and will sooner or later again fail inside some system headers.

Updated spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-20100428-1.23.p6.spkg

Upstream: Reported upstream. Developers acknowledge bug.

CC: @rishikesha

Component: packages: standard

Author: Volker Braun

Reviewer: David Kirkby

Merged: sage-4.7.alpha4

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

@vbraun vbraun added this to the sage-4.7 milestone Mar 8, 2011
@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Mar 30, 2011

comment:1

[Volker Braun wrote] The reason is the following code horror from src/src/include/Lcommon.h

The poor quality of the lcalc source code appears to have put off one Sage user when she looked at it. To quote her:

"''Unfortunately, lcalc was the part of Sage I intended using - now I think I
will look at other software.''"

http://groups.google.com/group/sage-support/browse_thread/thread/fab46afe7a8ac1c2/e43f31d452dfbfbe?lnk=gst&q=Warning+messages+when+compiling+%27lcalc%27#e43f31d452dfbfbe

I'm also very unimpressed. Even with your changes lcalc generates over 200 compiler warnings from gcc 4.6.0

Sun Studio would never compile lcalc (see #7065) - I doubt it will do even with your changes.

Lcalc used to refuse to compile on Solaris with g++ as the Makefile has a compiler option to suppress warnings from the assembler, which works with the GNU assembler, but which the Sun assembler does not understand. (The "-W" option was sent directly to the assembler with the g++ option "-Wa W".)

Lcalc will not install on HP-UX (#7178). I've never tried on AIX, but I would not be surprised if it failed on that too.

Your code builds OK with gcc 4.6.0 on OpenSolaris and passes all the doctests. So I'm assuming you want it reviewed, in which case I'll give it a positive review.

It would really help if you attached a Mercurial patch showing the changes you have made. It makes review easier, and is useful when people look back at tickets. I've attached a patch which shows your changes.

I've changed the Reported Upstream pull-down on trac from N/A to Not yet reported upstream. Will do shortly as clearly this is an upstream bug. I'll report it to
Micheal Rubinstein.

Dave

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Mar 30, 2011

Reviewer: David Kirkby

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Mar 30, 2011

Upstream: Not yet reported upstream; Will do shortly.

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Mar 30, 2011

Attachment: 10892-lcalc-fails-with-gcc.4.6.0.patch.gz

Patch for review purposes only - does not need to be applied.

@jdemeyer
Copy link

jdemeyer commented Apr 5, 2011

comment:4

Has this been reported upstream already?

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Apr 5, 2011

comment:5

Replying to @jdemeyer:

Has this been reported upstream already?

It had not before you reminded me, but I have now done it. I reported it to Michael Rubinstein (mrubinst << uwaterloo.ca) today @ 1307 GMT. I've changed the "reported upstream" pull-down to reflect this.

Dave

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Apr 5, 2011

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

@jdemeyer
Copy link

jdemeyer commented Apr 5, 2011

Merged: sage-4.7.alpha4

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Apr 5, 2011

Changed upstream from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Apr 5, 2011

comment:7

I got this from Mike a couple of hours ago, after I suggested he make his latest code available in it's current state, which is clearly not due for release yet.


I'll make that available to you later today, and thanks for keeping me updated about the compile issue.

Best,
Mike


@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:8

www.stp.dias.ie seems to be down, so I'm mirroring the spkg myself.

@jdemeyer
Copy link

jdemeyer commented May 4, 2011

Attachment: lcalc-SPKG.txt.diff.gz

Diff for SPKG.txt

@jdemeyer
Copy link

jdemeyer commented May 4, 2011

comment:9

New spkg with updated SPKG.txt, same place: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-20100428-1.23.p6.spkg

@jdemeyer
Copy link

comment:10

Any more feedback from upstream?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants