Fix remaining C++ issues of Lcalc (also to let it build with clang)

[ohanar]

Lcalc still uses some non-standard "features" of g++.

C++ compilers that are pickier than g++ will not allow this.

GCC 5.x again got stricter and now also needs the second "default parameter" patch (or once again -fpermissive) mentioned below.

---

Current branch fix usage of internal headers and some declarations. More to be done as I haven't even scratched the issues mentioned in this ticket - unless they have been fixed by another merged ticket.

[ohanar]

for review purposes

[leif]

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

We also should report this upstream.

Patch so far looks good (not sure whether I've seen all of it -- maybe trac truncated it); seems like most functions (also) had the default values in their declarations.

[leif]

GCC 4.9 now also requires -fpermissive to build Lcalc as well as the Cython extension modules that use Lcalc's headers, unless we fix Lcalc.

[leif]

I'll check [later^TM] whether Andrew's patch still applies (and whether it is sufficient for GCC 4.9 at least).

[leif]

Hmmm, haven't tested Andrew's patch [yet], but tried by myself.

For GCC 4.9, apparently the following tiny patch is sufficient to build Lcalc and the Sage library extension module (which currently lacks a depends btw.^*):

include/Ldirichlet_series.h

@@ -43 +43 @@
  //XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 template <class ttype>
 Complex L_function <ttype>::
-dirichlet_series(Complex s, long long N=-1)
+dirichlet_series(Complex s, long long N)
 {
     Complex z=0.;
     long long m,n;

include/L.h

@@ -491 +491 @@
 
     //#include "Ldirichlet_series.h" //for computing Dirichlet series
     Complex partial_dirichlet_series(Complex s, long long N1, long long N2);
-    Complex dirichlet_series(Complex s, long long N);
+    Complex dirichlet_series(Complex s, long long N=-1LL);
 
     //#include "Ltaylor_series.h" //for computing taylor series for Dirichlet series
     //void compute_taylor_series(int N, int K, Complex s_0, Complex *series);

(In addition to the patches to Lcalc we already have in Sage, of which only a trivial one is needed to build Lcalc outside of Sage.)

---

^*

src/module_list.py

@@ -700 +700 @@
 
     Extension('sage.libs.lcalc.lcalc_Lfunction',
               sources = ['sage/libs/lcalc/lcalc_Lfunction.pyx'],
-              libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx',
+              libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx',         # XXX Are all of these really needed? (NTL??)
                            'Lfunction'],
               include_dirs = [SAGE_INC + "/libLfunction"],
-              extra_compile_args=["-O3", "-ffast-math"],
+              depends = [ os.path.join(SAGE_INC, "libLfunction", header)
+                          for header in ["L.h"] ],                     # maybe more headers from there
+              extra_compile_args=["-O3", "-ffast-math"],               # probably add '-Wno-deprecated', as of Lcalc 1.23
               language = 'c++'),
 
 

(And, despite having language="c++", the extension module is compiled with the C compiler, not the C++ compiler, such that -- besides other effects -- CXXFLAGS aren't used. It gets linked with g++ though.)

[ohanar]

Your patch looks like a subset of mine. I can't remember why I did all the things that I did in my patch, but I would guess that I needed them to compile with clang. It may be that gcc 4.9 is still too permissive in this case.

[leif]

Replying to ohanar:

Your patch looks like a subset of mine.

Yes. My primary goal was to make Lcalc work with GCC; I'll take a closer look at other changes later.

Adding void to the friend declaration of reset() is certainly correct (although slightly unrelated to what the name of the patch suggests, on the broader topic of this ticket, namely making Lcalc's code build with clang).

In the other hunks, you just removed default parameters from definitions, but did not move them to corresponding declarations (probably because there aren't any, which is something I'll have to check). But from at least one comment, it looks like those parts were "dead" code anyway.

[leif]

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.

[thansen]

Hi leif,

these are quite some patches. Do you plan to send them upstream?

[leif]

Replying to thansen:

Hi leif,

these are quite some patches. Do you plan to send them upstream?

Well, upstream... ;-)

The patches are pretty small, and I'll merge them into fewer I think, hopefully next weekend.

Pretty a while ago, someone^TM wanted to create an autotools project for Michael, but that apparently never happened, so we keep heavily patching the existing Makefile.

AFAIK, some previous patches (e.g. dealing with newer PARI versions) went upstream, i.e., Michael should at least be aware of them; I'll certainly send him the new / updated ones later. Not sure whether he's interested in changes removing GNU extensions. (Variable-length arrays can still be used depending on macros / the compiler though.)

[thansen]

Well I hope upstreaming them works out. Sage is not the only downstream who needs these patches. There's at least Debian (where I'm maintaining the package) and Fedora. Having them upstream would make everyones live a lot easier.

[Snark]

I'm also interested in the issue. What is the status of the patches? Did upstream answer?

[leif]

GCC 5.x now also needs a small subset of the fixes for clang (or -fpermissive; then also a couple of Sage extension modules which include Lcalc headers have to get built with -fpermissive, in CFLAGS(!)). (cf. ticket description)

I'll probably deal with that on another ticket.

[fbissey]

The -fpermissive bit could live in module_list.py with the appropriate sage extensions. But can the headers be fixed?

[leif]

Replying to fbissey:

The -fpermissive bit could live in module_list.py with the appropriate sage extensions. But can the headers be fixed?

Sure, trivial for g++ at least.

We of course could also add -fpermissive in Lcalc's spkg-install, but I rather meant

env CXXFLAGS="-fpermissive" ./sage -f lcalc
env CFLAGS="-fpermissive" ./sage -b

to temporarily work around the errors when building Sage (with GCC 5.x).

I had two other issues (with gf2x and ncurses) which hopefully vanish tomorrow [i.e., later today down under] when the final 5.1 (or 5.0.1?) gets released.

[leif]

Replying to fbissey:

The -fpermissive bit could live in module_list.py with the appropriate sage extensions. But can the headers be fixed?

I probably misunderstood you; the (GCC 5.x/default parameters) patch is the same for Lcalc and the headers the Sage library uses (i.e., Lcalc uses the same so the errors are exactly the same, one patch for both issues).

[fbissey]

I think I had misunderstood your description. Final release of gcc will be 5.1. X.0.x are now pre-release. In effect betas and release candidates. Was a pain in the ass to build 5.0.1_rc on my cluster, there is regression in the build system, using --with-gmp= and friends can bring system headers that you don't want in phase 2 and 3. Happened in the past and been fixed and now it's back,

[leif]

This looks like the second default parameters patch I mentioned:

lcalc-1.23/include/Lgamma.h

@@ -77 +77 @@
 //n=0 should just give log_GAMMA(z)... thus making log_GAMMA
 //code obsolete. But leave log_GAMMA intact anyways.
 template <class ttype>
-precise(ttype) log_GAMMA (ttype z,int n=0)
+precise(ttype) log_GAMMA (ttype z,int n)
 {
     int M;
     precise(ttype) log_G,r,r2,y;
@@ -230 +230 @@
 //value exp_w which holds exp(-w)
 //computes G(z,w), so there's an extra w^(-z) factor.
 template <class ttype>
-Complex inc_GAMMA (ttype z,ttype w, const char *method="temme", ttype exp_w = 0, bool recycle=false)
+Complex inc_GAMMA (ttype z,ttype w, const char *method, ttype exp_w, bool recycle)
 {
 
     Complex G;
@@ -334 +334 @@
 
 
 template <class ttype>
-ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w=0, bool recycle=false)  //computes G(z,w) via continued fraction
+ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle)  //computes G(z,w) via continued fraction
 {
 
         ttype G;
@@ -424 +424 @@
 }
 
 template <class ttype>
-ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w = 0, bool recycle=false)  //computes G(z,w) via asymptotic series
+ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle)  //computes G(z,w) via asymptotic series
 {
 
         if(my_verbose>3) cout << "called asympt_GAMMA("<<z<<","<<w<<")"<< endl;
@@ -446 +446 @@
 
 
 template <class ttype>
-ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w = 0, bool recycle=false)  //computes g(z,w)
+ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w, bool recycle)  //computes g(z,w)
 {
 
     ttype g;
@@ -604 +604 @@
 }
 
 template <class ttype>
-Complex gamma_sum(Complex s, int what_type, ttype *coeff, int N, Double g, Complex l, Double Q, Long Period, Complex delta=1, const char *method="temme")
+Complex gamma_sum(Complex s, int what_type, ttype *coeff, int N, Double g, Complex l, Double Q, Long Period, Complex delta, const char *method)
 {
     Complex SUM=0;
 

(Not yet checked whether it's complete/sufficient; the GCC 5.0.1 release candidate is on another machine.)

[leif]

Replying to fbissey:

I think I had misunderstood your description. Final release of gcc will be 5.1. X.0.x are now pre-release. In effect betas and release candidates. Was a pain in the ass to build 5.0.1_rc on my cluster, there is regression in the build system, using --with-gmp= and friends can bring system headers that you don't want in phase 2 and 3. Happened in the past and been fixed and now it's back,

I was surprised building the first RC went smooth (on a pretty "clean" machine though); this time no cloog/PPL/ISL version hazards or the like... (make clean still bails out though IIRC.)

W.r.t. the versioning: 5.1 in the subject, 5.0.1 in the message body; RC1 tarball called 5.1.0-, identifies as 5.0.1 (prerelease)... B)

[leif]

The ​lcalc-1.23_default_parameters_2.patch is now ​there, and sufficient for GCC 5.x (needs review).

[jdemeyer]

Can this be closed as duplicate of #18316?

[leif]

Replying to jdemeyer:

Can this be closed as duplicate of #18316?

Nope, we just need a good new title (as the other issues with clang / C++, cf. the comment above) have been discussed here as well... (There's also #12436, but this one is probably a bit fresher.)

[leif]

P.S.:

I also have to retest my patches with more recent versions of clang and g++.

[fbissey]

Leif, can you put the remaining stuff in a branch to test? And I would say close #12436 as a duplicate.

[fbissey]

New commits:

​8a9fe64

Remove use of internal header- cmath should be sufficient. Fix declaration.

[fbissey]

Next issue

[lcalc-1.23.p14] clang++ -O3   -ffast-math -fPIC   -I../include -c Lriemannsiegel.cc
[lcalc-1.23.p14] In file included from Lriemannsiegel.cc:24:
[lcalc-1.23.p14] In file included from ../include/L.h:539:
[lcalc-1.23.p14] ../include/Ldokchitser.h:72:14: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>')
[lcalc-1.23.p14]     Complex m[N+1];
[lcalc-1.23.p14]              ^
[lcalc-1.23.p14] ../include/Ldokchitser.h:81:23: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>')
[lcalc-1.23.p14]         Complex sum_log_Gamma[N+1][MYDIGITS+1];
[lcalc-1.23.p14]                              ^
[lcalc-1.23.p14] ../include/Ldokchitser.h:106:27: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>')
[lcalc-1.23.p14]         Complex exp_sum_log_Gamma[N+1][MYDIGITS+1][MYDIGITS+1]; // symmetric functions
[lcalc-1.23.p14]                                  ^
[lcalc-1.23.p14] ../include/Ldokchitser.h:107:15: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>')
[lcalc-1.23.p14]         Complex gamma[N+1][MYDIGITS+1]; // gamma(s+m[j]) for j = 1 to N
[lcalc-1.23.p14]                      ^
[lcalc-1.23.p14] In file included from Lriemannsiegel.cc:24:
[lcalc-1.23.p14] In file included from ../include/L.h:542:
[lcalc-1.23.p14] ../include/Lexplicit_formula.h:28:12: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>')
[lcalc-1.23.p14]   Complex b[num_coeffs+1];
[lcalc-1.23.p14]            ^
[lcalc-1.23.p14] 5 errors generated.

[fbissey]

The issue I hit now with this is pretty much the VLA. I am guessing I can get the first VLA patch from #12436 but not the second one.

@leif do you still have those patches and can you share them?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​f498bff

Adapt old patch from Andrew Ohana for VLA in C++

[fbissey]

Actually doing the VLA like Andrew did initially in #12436 is good enough. And while we have warnings for all the other stuff mentioned in this tickets it all compiled successfully with clang (latest on Sierra, based on 3.8).

[tscrim]

I'm hesitant about the changes of the form:

+diff --git a/include/Ldokchitser.h b/include/Ldokchitser.h
+index c67f01a..7b3e5c9 100644
+--- a/include/Ldokchitser.h
++++ b/include/Ldokchitser.h
+@@ -69,7 +69,7 @@ phi_series(int precision)
+     
+     // compute the values m[j] for the respective lambda_k[j]
+     
+-    Complex m[N+1];
++    std::vector<Complex> m(N+1);
+     for (j=1;j<=N;j++)
+       m[j] = -2*lambda_k[j] + 2;
+       

because my understanding is g++ to treats Complex m[N+1] as Complex *m = new Complex[N+1], which changing to std::vector could have an impact on performance. I haven't looked at the code itself, but my guess is that lcalc wanted to treat these as fixed length arrays whose size is determined at runtime.

[fbissey]

Correct, they are VLA. The replacement with new works very well for one dimensional arrays. Multidimensional ones are another story, I guess it would be similar with more ugly loops. It seems to me that the accepted behavior in C++ is to use vector for these (from stack overflow examples). The mystery to me is that there are many more VLA in that particular code but only a few of them raise a flag. What gives?

[tscrim]

I can understand why std::vector would be a natural choice because you don't have to worry about memory management (specifically the deallocation). It just there is some slight overhead from using it rather than direct system calls with the array, but that is more likely a micro-optimization. Perhaps some of the others aren't raising warnings because their length can be determined at compile time?

One little change to a patch:

         std::vector<std::vector<std::vector<Complex> > > exp_sum_log_Gamma(N+1); // symmetric functions
         std::vector<std::vector<Complex> > gamma(N+1); // gamma(s+m[j]) for j = 1 to N
         for (j=1;j<=N;j++){
             exp_sum_log_Gamma[j].resize(MYDIGITS+1);
+            for (n=0;n<=MYDIGITS;n++) exp_sum_log_Gamma[j][n].resize(MYDIGITS+1);
             gamma[j].resize(MYDIGITS+1);
         }
-        for (j=1;j<=N;j++) for (n=0;n<=MYDIGITS;n++) exp_sum_log_Gamma[j][n].resize(MYDIGITS+1);

as I think it is (slightly) more readable considering we are already working with the 3d array.

[fbissey]

I wouldn't worry too much about optimization at this stage. The compiler does a better job than you'll ever do on that. The only thing the compiler cannot do is find a better algorithm (although it may be able able to re-order your loops in a way you wouldn't have thought of). For the little change I copied the structure of the original patch which does that in separate stages. If you feel it is clearer your way, feel free to commit your change as a part of your review (I can do it myself if you insist).

[tscrim]

It's in the realm of bikeshedding, so I'm sending this off to the buildbots.