Opened 10 years ago

Closed 5 years ago

#12439 closed defect (fixed)

symmetrica fails to compile with clang

Reported by: ohanar Owned by: GeorgSWeber
Priority: major Milestone: sage-7.4
Component: packages: standard Keywords:
Cc: tscrim, fbissey 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 fbissey)

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 jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by jdemeyer

Please fill in your real name as Author.

comment:3 Changed 9 years ago by jdemeyer

  • Component changed from build to packages

Changed 9 years ago by jdemeyer

comment:4 Changed 9 years ago by jdemeyer

  • Authors set to R. Andrew Ohana, Jeroen Demeyer
  • Description modified (diff)

comment:5 Changed 9 years ago by jdemeyer

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

comment:6 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

Andrew, do you feel like reviewing this?

comment:8 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_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 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:12 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 5 years ago by tscrim

  • Cc tscrim added
  • Milestone changed from sage-6.4 to sage-7.4

comment:14 Changed 5 years ago by tscrim

  • Authors changed from R. Andrew Ohana, Jeroen Demeyer to R. Andrew Ohana, Jeroen Demeyer, Travis Scrimshaw
  • Branch set to public/packages/symmetrica_clang-12439
  • Cc fbissey added
  • Commit set to 2133a16b4ee8c8d363f3cc44caaf33086b9b4ae8
  • Status changed from needs_work to needs_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 5 years ago by fbissey

  • Status changed from needs_review to needs_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 5 years ago by git

  • Commit changed from 2133a16b4ee8c8d363f3cc44caaf33086b9b4ae8 to 41f77fbdc6970abf6685cb7a32b55298b2bbc5d9

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 5 years ago by tscrim

  • Status changed from needs_work to needs_review

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

comment:18 Changed 5 years ago by fbissey

  • Authors changed from R. Andrew Ohana, Jeroen Demeyer, Travis Scrimshaw to Travis 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 5 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Good to go.

comment:20 Changed 5 years ago by tscrim

Thank you.

comment:21 Changed 5 years ago by vbraun

  • Branch changed from public/packages/symmetrica_clang-12439 to 41f77fbdc6970abf6685cb7a32b55298b2bbc5d9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.