Opened 11 years ago

Closed 6 years ago

#12439 closed defect (fixed)

symmetrica fails to compile with clang

Reported by: R. Andrew Ohana Owned by: Georg S. Weber
Priority: major Milestone: sage-7.4
Component: packages: standard Keywords:
Cc: Travis Scrimshaw, François Bissey Merged in:
Authors: Travis Scrimshaw Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 41f77fb (Commits, GitHub, GitLab) Commit: 41f77fbdc6970abf6685cb7a32b55298b2bbc5d9
Dependencies: Stopgaps:

Status badges

Description (last modified by François Bissey)

There is one function with empty return statements when an int should be returned. Clang doesn't like this.

Attachments (1)

symmetrica-2.0.p8.diff (1.7 KB) - added by Jeroen Demeyer 10 years ago.

Download all attachments as: .zip

Change History (22)

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

Status: newneeds_review

comment:2 Changed 10 years ago by Jeroen Demeyer

Please fill in your real name as Author.

comment:3 Changed 10 years ago by Jeroen Demeyer

Component: buildpackages

Changed 10 years ago by Jeroen Demeyer

Attachment: symmetrica-2.0.p8.diff added

comment:4 Changed 10 years ago by Jeroen Demeyer

Authors: R. Andrew Ohana, Jeroen Demeyer
Description: modified (diff)

comment:5 Changed 10 years ago by Jeroen Demeyer

Changed Wreturn-type to Wno-return-type :-) It now builds with Clang 3.2.

comment:6 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:7 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.10sage-5.11

Andrew, do you feel like reviewing this?

comment:8 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:9 Changed 9 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

This needs to rebased to the latest Symmetrica spkg. Since nobody seems to care, I'm not going to do that.

comment:10 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:11 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:12 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:13 Changed 6 years ago by Travis Scrimshaw

Cc: Travis Scrimshaw added
Milestone: sage-6.4sage-7.4

comment:14 Changed 6 years ago by Travis Scrimshaw

Authors: R. Andrew Ohana, Jeroen DemeyerR. Andrew Ohana, Jeroen Demeyer, Travis Scrimshaw
Branch: public/packages/symmetrica_clang-12439
Cc: François Bissey added
Commit: 2133a16b4ee8c8d363f3cc44caaf33086b9b4ae8
Status: needs_workneeds_review

Give this a try. The function that was marked with an int return type really should have been a void. I've added the compiler flag to the branch as well.


New commits:

2133a16Adding patch for function with wrong return type.

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

Status: needs_reviewneeds_work

I am afraid I cannot accept that. You are mistaken in thinking the function should have been void

static int rec01(INT ni, OP vec)
/* to compute number of partitions */
{
    INT erg = OK;
    if (ni<0) return;
    if (not EMPTYP(S_V_I(vec,ni))) return;
    else if (ni<=1) M_I_I(1,S_V_I(vec,ni));
    else {

        INT m,og;
        og = ni/3+3;
        m_i_i(0,S_V_I(vec,ni));

        for (m=1;m<og;m++)
        {
            INT j;
            j = ni-m*(3*m-1)/2;
            if (j<0) break;
            if (m%2==0) { ADDINVERS_APPLY(S_V_I(vec,j));
                          ADD_APPLY(S_V_I(vec,j),S_V_I(vec,ni));
                          ADDINVERS_APPLY(S_V_I(vec,j));
                        }
            else          ADD_APPLY(S_V_I(vec,j),S_V_I(vec,ni));
            j = ni-m*(3*m+1)/2;
            if (j<0) break;
            if (m%2==0) { ADDINVERS_APPLY(S_V_I(vec,j));
                          ADD_APPLY(S_V_I(vec,j),S_V_I(vec,ni));
                          ADDINVERS_APPLY(S_V_I(vec,j));
                        }
            else          ADD_APPLY(S_V_I(vec,j),S_V_I(vec,ni));
            }
        }
    ENDR("internal:rec01");
}

The error here is in thinking that there is only a couple of instances of return; in the function and that you reach the end of it without a return. But ENDR is a macro defined in macro.h

#define ENDR(a) endr_ende: if (erg != OK) EDC(a); return erg;

So you have a final return of an integer value. It is just hidden. By default gcc converts the return; statements to return 0; in that case and that's what we should do.

And we don't want to add that flag to spkg-install.

comment:16 Changed 6 years ago by git

Commit: 2133a16b4ee8c8d363f3cc44caaf33086b9b4ae841f77fbdc6970abf6685cb7a32b55298b2bbc5d9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

41f77fbAdded patch for return values of symmetrica.

comment:17 Changed 6 years ago by Travis Scrimshaw

Status: needs_workneeds_review

My fault, I should have read deeper into the code. Fixed.

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

Authors: R. Andrew Ohana, Jeroen Demeyer, Travis ScrimshawTravis Scrimshaw
Description: modified (diff)

All forgiven :) I am doing a new run of compiling with clang (3.8.0 I have upgraded to sierra and xcode 8.0) on my mac. I'll put that in, that should be the proverbial proof of the pudding.

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

Reviewers: François Bissey
Status: needs_reviewpositive_review

Good to go.

comment:20 Changed 6 years ago by Travis Scrimshaw

Thank you.

comment:21 Changed 6 years ago by Volker Braun

Branch: public/packages/symmetrica_clang-1243941f77fbdc6970abf6685cb7a32b55298b2bbc5d9
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.