libantioch / antioch

C++ Chemical Kinetics, Thermodynaimics, and Transport Library
https://libantioch.github.io/
Other
23 stars 17 forks source link

Add more universal usage of auto/decltype #10

Closed pbauman closed 11 years ago

pbauman commented 11 years ago

7 had some discussion of using this for the VexCL interface, but there's plenty of other places we should be able to use it.

@roystgnr started down this road in the auto_decltype branch. Wanted to open this issue to track discussion.

@roystgnr, I just successfully built against 62c4bc9 using OS X clang (in c++11 mode). Correspondingly confirmed build failure with gcc 4.7.3 on OS X

In file included from ../../../master/src/transport/include/antioch/mixture_viscosity.h:33:0,
                 from ../../../master/src/parsing/include/antioch/blottner_parsing.h:34,
                 from ../../../master/src/parsing/src/blottner_parsing.C:33:
../../../master/src/core/include/antioch/chemical_mixture.h: In instantiation of 'class Antioch::ChemicalMixture':
../../../master/src/parsing/include/antioch/blottner_parsing.h:76:6:   required from 'void Antioch::read_blottner_data_ascii(Antioch::MixtureViscosity, NumericType>&, std::istream&) [with NumericType = double; std::istream = std::basic_istream]'
../../../master/src/parsing/src/blottner_parsing.C:43:26:   required from here
../../../master/src/core/include/antioch/chemical_mixture.h:253:3: error: too few arguments to function

For posterity, clang version is

Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
Target: x86_64-apple-darwin12.4.0
Thread model: posix

I'll build gcc 4.8.1 and post results from that as well.

roystgnr commented 11 years ago

So here's the problem:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54359

And it's just been fixed in gcc 4.8.1

roystgnr commented 11 years ago

Which leaves us with two options I can see:

Make the header files arguably uglier by pulling all our function definitions inside the class definition, and thereby get compatibility again back to gcc 4.6.something.

Disable C++11 auto support with everything prior to gcc 4.8.1

I'm leaning toward the first option, but I'd be fine with being outvoted and I'd be thrilled if there's a better third option that I haven't thought of.

pbauman commented 11 years ago

There's no arguing that the header would be uglier. ;-)

That said, I can't see a third alternative. I can see it both ways with this. I'd prefer the latter (restrict C++11 to gcc4.8.1+/clang/etc), I think, since all the supercomputing systems will have older compilers anyway so it won't make a difference, we'll be without C++11. I would change my mind I think if by moving the function definitions into the header would get us compiling with Intel; that would be a pretty compelling I think.

roystgnr commented 11 years ago

My only problem with gcc 4.8.1 is that it's literally less than two months old and there are no other versions that would work. If we run into a regression in gcc (or as has been common for me in the past, a tightening of standards which breaks not-quite-C++-compliant third party code) then we end up out of luck.

On the other hand, I guess clang would make a good backup; I can get our Sysnet modules up to date and add it to the buildbot tests. And when icpc finally meets our C++11 requirements it'll be very unlikely that they'll happen to match an old gcc bug.

That settles it, then. I'll write an autoconf test for this bug (or get lazy and just check gcc version numbers when we have a real gcc) and I'll set the auto/decltype macros to C++03 mode if the bug is present. We'll just need to remember to always run our benchmarks with the right compilers.

pbauman commented 11 years ago

OK, cool. I'm tinkering with building the newest clang. Once I'm run through the tree of libraries on my laptop, I'll start on some modules for ICES.

roystgnr commented 11 years ago

So we work fine with 4.8.1; I haven't added the autoconf test yet to the branch yet though.

Something I just ran into:

There doesn't seem to be a way to do "using namespace std::" inside a decltype(), which means we can't do the usual "using std::exp; return exp(T);" tricks to get std::exp when applicable and Koenig lookup when applicable. Would anyone mind if I just make inline Antioch:: wrappers for all the cmath functions? A side benefit would be that we could easily turn https://code.google.com/p/fastapprox/ or other fast-but-inaccurate approximation alternatives on and off for benchmarking.

pbauman commented 11 years ago

Fine with me. Let me know if you want some help.

roystgnr commented 11 years ago

Update: the cmath_shims.h trick works for getting everything to compile, but I'm now seeing a segfault in "make check" when operator= tries to handle a valarray expression template from ChemicalMixture::X. It almost looks like a reference-to-temporary bug in the valarray headers, but despite my recent thrilling "I argued with the compiler and the compiler was wrong moment" I still have a strong prior in favor of the "my code is wrong even when it looks like a compiler bug" class of theories...

pbauman commented 11 years ago

FWIW, applying the patch below (needed to get compilation) and using no optional libraries, make check passes for me with Clang 3.3/libcxx 3.3 on the OS X laptop. (Sorry for posting it in the ticket - GitHub won't let me attach it...)

I'll build a copy of GCC 4.8.1 on the laptop and see if I can replicate (will take a few hours probably).

diff --git a/test/arrhenius_rate_vec_unit.C b/test/arrhenius_rate_vec_unit.C
index 8b0974e..07a59a2 100644
--- a/test/arrhenius_rate_vec_unit.C
+++ b/test/arrhenius_rate_vec_unit.C
@@ -55,7 +55,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/blottner_viscosity_vec_unit.C b/test/blottner_viscosity_vec_unit.C
index 7be634a..bc30711 100644
--- a/test/blottner_viscosity_vec_unit.C
+++ b/test/blottner_viscosity_vec_unit.C
@@ -59,7 +59,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/cea_evaluator_vec_unit.C b/test/cea_evaluator_vec_unit.C
index d219048..09c764c 100644
--- a/test/cea_evaluator_vec_unit.C
+++ b/test/cea_evaluator_vec_unit.C
@@ -57,7 +57,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/cea_thermo_vec_unit.C b/test/cea_thermo_vec_unit.C
index cdfc8fa..ea0c285 100644
--- a/test/cea_thermo_vec_unit.C
+++ b/test/cea_thermo_vec_unit.C
@@ -56,7 +56,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/chem_mixture_vec_unit.C b/test/chem_mixture_vec_unit.C
index 5992574..4918763 100644
--- a/test/chem_mixture_vec_unit.C
+++ b/test/chem_mixture_vec_unit.C
@@ -63,7 +63,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif
 #include "antioch/vector_utils.h"

 #ifdef ANTIOCH_HAVE_GRVY
diff --git a/test/kinetics_regression_vec.C b/test/kinetics_regression_vec.C
index 982f1b5..3875678 100644
--- a/test/kinetics_regression_vec.C
+++ b/test/kinetics_regression_vec.C
@@ -63,7 +63,9 @@
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
 #include "antioch/vector_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/kinetics_vec_unit.C b/test/kinetics_vec_unit.C
index b7267a8..3f4bafd 100644
--- a/test/kinetics_vec_unit.C
+++ b/test/kinetics_vec_unit.C
@@ -63,7 +63,9 @@
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
 #include "antioch/vector_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/mixture_viscosity_vec_unit.C b/test/mixture_viscosity_vec_unit.C
index 8935143..6727ede 100644
--- a/test/mixture_viscosity_vec_unit.C
+++ b/test/mixture_viscosity_vec_unit.C
@@ -59,7 +59,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/sutherland_viscosity_vec_unit.C b/test/sutherland_viscosity_vec_unit.C
index 463e4d1..531ce8a 100644
--- a/test/sutherland_viscosity_vec_unit.C
+++ b/test/sutherland_viscosity_vec_unit.C
@@ -61,7 +61,9 @@
 #include "antioch/eigen_utils.h"
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
diff --git a/test/wilke_transport_vec_unit.C b/test/wilke_transport_vec_unit.C
index 6181ebc..db761a0 100644
--- a/test/wilke_transport_vec_unit.C
+++ b/test/wilke_transport_vec_unit.C
@@ -60,7 +60,9 @@
 #include "antioch/metaphysicl_utils.h"
 #include "antioch/valarray_utils.h"
 #include "antioch/vector_utils.h"
+#ifdef ANTIOCH_HAVE_VEXCL
 #include "antioch/vexcl_utils.h"
+#endif

 #ifdef ANTIOCH_HAVE_GRVY
 #include "grvy.h"
pbauman commented 11 years ago

OK, using GCC 4.8.1 with no optional libraries, the code compiles. Only the chem_mixture_vec_unit and arrhenius_rate_vec_unit die, everything else passes. There was a warning emitted when the tests compiled:

In file included from ../../../master/test/chem_mixture_vec_unit.C:59:0:
../../../master/src/core/include/antioch/chemical_mixture.h: In member function ‘void Antioch::ChemicalMixture::X(typename Antioch::value_type::type, const VectorStateType&, VectorStateType&) const’:
../../../master/src/core/include/antioch/chemical_mixture.h:388:50: warning: typedef ‘StateType’ locally defined but not used [-Wunused-local-typedefs]
       Antioch::value_type::type StateType;
roystgnr commented 11 years ago

Closing issue #10 since it ought to be (eventually) fully handled by the pull request in issue #12