Ticket #10892 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

lcalc fails to build with gcc-4.6

Reported by: vbraun Owned by: tbd
Priority: major Milestone: sage-4.7
Component: packages: standard Keywords:
Cc: Rishikesh Work issues:
Report Upstream: Reported upstream. Developers acknowledge bug. Reviewers: David Kirkby
Authors: Volker Braun Merged in: sage-4.7.alpha4
Dependencies: Stopgaps:

Description (last modified by jdemeyer) (diff)

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

Attachments

10892-lcalc-fails-with-gcc.4.6.0.patch Download (5.6 KB) - added by drkirkby 2 years ago.
Patch for review purposes only - does not need to be applied.
lcalc-SPKG.txt.diff Download (2.8 KB) - added by jdemeyer 2 years ago.
Diff for SPKG.txt

Change History

comment:1 Changed 2 years ago by drkirkby

  • Reviewers set to David Kirkby
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

[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

Changed 2 years ago by drkirkby

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

comment:2 Changed 2 years ago by drkirkby

  • Status changed from new to needs_review

comment:3 Changed 2 years ago by drkirkby

  • Status changed from needs_review to positive_review

comment:4 follow-up: ↓ 5 Changed 2 years ago by jdemeyer

Has this been reported upstream already?

comment:5 in reply to: ↑ 4 Changed 2 years ago by drkirkby

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

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 <<<ATT> uwaterloo.ca) today @ 1307 GMT. I've changed the "reported upstream" pull-down to reflect this.

Dave

comment:6 Changed 2 years ago by jdemeyer

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-4.7.alpha4

comment:7 Changed 2 years ago by drkirkby

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

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


comment:8 Changed 2 years ago by jdemeyer

  • Description modified (diff)

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

Changed 2 years ago by jdemeyer

Diff for SPKG.txt

comment:9 Changed 2 years ago by jdemeyer

comment:10 Changed 2 years ago by jdemeyer

Any more feedback from upstream?

Note: See TracTickets for help on using tickets.