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:  sage7.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: 
Description (last modified by )
There is one function with empty return statements when an int should be returned. Clang doesn't like this.
Attachments (1)
Change History (22)
comment:1 Changed 11 years ago by
Status:  new → needs_review 

comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
Component:  build → packages 

Changed 10 years ago by
Attachment:  symmetrica2.0.p8.diff added 

comment:4 Changed 10 years ago by
Authors:  → R. Andrew Ohana, Jeroen Demeyer 

Description:  modified (diff) 
comment:5 Changed 10 years ago by
Changed Wreturntype
to Wnoreturntype
:) It now builds with Clang 3.2.
comment:6 Changed 10 years ago by
Description:  modified (diff) 

comment:7 Changed 10 years ago by
Milestone:  sage5.10 → sage5.11 

Andrew, do you feel like reviewing this?
comment:8 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:9 Changed 9 years ago by
Status:  needs_review → 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 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:11 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:12 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:13 Changed 6 years ago by
Cc:  Travis Scrimshaw added 

Milestone:  sage6.4 → sage7.4 
comment:14 Changed 6 years ago by
Authors:  R. Andrew Ohana, Jeroen Demeyer → R. Andrew Ohana, Jeroen Demeyer, Travis Scrimshaw 

Branch:  → public/packages/symmetrica_clang12439 
Cc:  François Bissey added 
Commit:  → 2133a16b4ee8c8d363f3cc44caaf33086b9b4ae8 
Status:  needs_work → 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:
2133a16  Adding patch for function with wrong return type.

comment:15 Changed 6 years ago by
Status:  needs_review → 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 = nim*(3*m1)/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 = nim*(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 spkginstall
.
comment:16 Changed 6 years ago by
Commit:  2133a16b4ee8c8d363f3cc44caaf33086b9b4ae8 → 41f77fbdc6970abf6685cb7a32b55298b2bbc5d9 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
41f77fb  Added patch for return values of symmetrica.

comment:17 Changed 6 years ago by
Status:  needs_work → needs_review 

My fault, I should have read deeper into the code. Fixed.
comment:18 Changed 6 years ago by
Authors:  R. Andrew Ohana, Jeroen Demeyer, Travis Scrimshaw → 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 6 years ago by
Reviewers:  → François Bissey 

Status:  needs_review → positive_review 
Good to go.
comment:21 Changed 6 years ago by
Branch:  public/packages/symmetrica_clang12439 → 41f77fbdc6970abf6685cb7a32b55298b2bbc5d9 

Resolution:  → fixed 
Status:  positive_review → closed 
Please fill in your real name as Author.