Opened 11 years ago

Closed 6 years ago

#12436 closed defect (invalid)

lcalc uses variable length arrays

Reported by: R. Andrew Ohana Owned by: Georg S. Weber
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12681 Stopgaps:

Status badges

Description (last modified by R. Andrew Ohana)

This is an issue for building on non-gnu compilers. While not perfect, a solution for this is to use std::vector.

I've posted an spkg with this solution (fixes #12435, #12437) at http://wstein.org/home/ohanar/clang-port/sage-5.0.beta1-src/spkg/standard/lcalc-1.23.p10.spkg. (This is a review spkg in case there are more issues with clang down the road.)

Attachments (1)

lcalc-variable-length.patch (2.4 KB) - added by R. Andrew Ohana 11 years ago.
for review purposes

Download all attachments as: .zip

Change History (16)

Changed 11 years ago by R. Andrew Ohana

Attachment: lcalc-variable-length.patch added

for review purposes

comment:1 Changed 11 years ago by R. Andrew Ohana

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 11 years ago by R. Andrew Ohana

Description: modified (diff)

comment:3 Changed 11 years ago by Leif Leonhardy

Dependencies: 12435#12681
Status: needs_reviewneeds_work

We now need a new spkg (based on that of #12681) incorporating this patch.

Wonder whether we should submit it upstream; I personally would (currently) only apply the patch if $CC is not GCC.

comment:4 Changed 11 years ago by Leif Leonhardy

Work issues: Create a new spkg.

comment:5 in reply to:  3 Changed 11 years ago by Leif Leonhardy

Replying to leif:

Wonder whether we should submit it upstream; I personally would (currently) only apply the patch if $CC is not GCC.

By now, ... if $CXX is not GCC's C++ compiler.

(We shouldn't literally check $CXX for g++, but instead do something like if ! $CXX --version | grep g++ >/dev/null; then # apply patch. Apparently the output depends of the filename of the executable btw., which shouldn't hurt since applying the patch is ok for g++ as well; looking for "GCC" in contrast could give false positives, since some other compilers might report with what they've been built, too.)

Last edited 11 years ago by Leif Leonhardy (previous) (diff)

comment:6 in reply to:  3 Changed 11 years ago by R. Andrew Ohana

Replying to leif:

Wonder whether we should submit it upstream; I personally would (currently) only apply the patch if $CC is not GCC.

I haven't really pushed this ticket (which I think is important) since it is significant enough change to report upstream, which I have yet to get around to.

I also think that the makefile patch you've made should be submitted to upstream.

comment:7 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:8 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:9 Changed 9 years ago by Jeroen Demeyer

Component: buildpackages: standard

comment:10 Changed 8 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:11 Changed 8 years ago by Leif Leonhardy

Ok, so I've now created 7 new patches to fix all remaining clang/non-GNU/C++11 issues with Lcalc (except for some deprecation warnings):

Makefile_2-dont_compile_C_with_CXXFLAGS.patch
lcalc-1.23-fix_VLA_1.patch
lcalc-1.23-fix_VLA_2.patch
lcalc-1.23-fix_control_reaches_end_of_non-void_fn.patch
lcalc-1.23-fix_typeof.patch
lcalc-1.23-missing_return_type.patch
lcalc-1.23_default_parameters_2.patch

But I'd prefer to put them on a single ticket, i.e., to either open a new one, or repurpose one of the two existing ones. (But before doing so, I'll anyway polish them a bit, and probably merge some of them into another or into one of the existing ones.)

I did take a slightly different approach to variable-length arrays; instead of using std::vector<>, I'm using foo_t *_foo = new foo_t[dim1*dim2*dim3], along with a macro foo(i,j,k) which maps the indices to a single one, and of course added delete [] _foo where necessary.

So far tested with GCC 4.4, 4.8 and 4.9, clang 3.4.1, and a couple of -std=... variations.

comment:12 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:13 Changed 7 years ago by Leif Leonhardy

We should probably fix the remaining issues (including VLAs) on #12437, and close this ticket as a duplicate.

Or fix VLAs here, and let #12437 depend on this ticket.

comment:14 Changed 6 years ago by François Bissey

Milestone: sage-6.4sage-duplicate/invalid/wontfix
Reviewers: François Bissey
Status: needs_workpositive_review
Work issues: Create a new spkg.

Obsolete and all useful content used in #12437.

comment:15 Changed 6 years ago by Volker Braun

Resolution: invalid
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.