search

OpenGATE / Gate

Official public repository of Gate
http://www.opengatecollaboration.org
GNU Lesser General Public License v3.0
236 stars 263 forks source link

Gate C++11 #22

Closed astuden100 closed 9 years ago

astuden100 commented 9 years ago

New versions of ROOT are c+11 based, and GATE is not yet fully c+11 compatible.

I was able to compile using the following code modifications:

1.) I had to add -std=c++11 to CMAKE_CXX_FLAGS. In ccmake window toggle to advanced mode via [t] and add the flag, then re-configure [c] and then generate [g] before re-running make.

2.) ifstream and ofstream are not prepended by std:: as they should be without a global using namespace std. I used replace, e.g.:

#> replace ofstream std::ofstream -- source/physics/src/GateCrossSectionsTable.cc

3.) Since I used c++11 standard of c++, some CLHEP files assumed I want Thread Local Storage (TLS) enabled, which caused a mismatch between the compiled version of CLHEP (TLS disabled) and Gate portion of CLHEP (TLS enabled). To resolve, I replaced

source/externals/clhep/include/CLHEP/Utility/thread_local.h

with a new thread_local.h where CLHEP_THREAD_LOCAL is defined to an empty string:

define CLHEP_THREAD_LOCAL

UPDATE: Pull request #21 of Gate on git is supposed to be c++11 compatible. Didn't have time to verify it yet.

Andrej

BishopWolf commented 9 years ago

I want to understand the implications of point 3, without that change I am able to run Gate against c++11. So what is the point here??

astuden100 commented 9 years ago

On 05/27/15 12:50, Alex Vergara Gil wrote:

I want to understand the implications of point 3, without that change I am able to run Gate against c++11. So what is the point here??

— Reply to this email directly or view it on GitHub https://github.com/OpenGATE/Gate/issues/22#issuecomment-105865250.

Hi Alex!

It is just a system consistency related issue. In my case the base OS and set of basic applications is maintained centrally and distributed across several PCs which are used for very different purposes, including pure HEP. It just so happened, that libCLHEP was compiled without TLS, and I am not really all that interested to know why that is so. On the other hand, Gate uses a very particular query to estimate whether to use TLS or not (see below). It can certainly happen that it guesses the correct value for CLHEP_THREAD_LOCAL, and you will not see the issue I posted. It may, however, fail. In that case, use point 3.) :-)

Quoting [source/externals/clhep/include/CLHEP/Utility/thread_local.h]

if __cplusplus >= 201103L

if GNUC > 4 || (GNUC == 4 && GNUC_MINOR > 7)

 #define CLHEP_THREAD_LOCAL thread_local

elif clang

 #if __has_feature(cxx_thread_local)
   #define CLHEP_THREAD_LOCAL thread_local
 #else
   #define CLHEP_THREAD_LOCAL
 #endif

else

 #define CLHEP_THREAD_LOCAL

endif

else

define CLHEP_THREAD_LOCAL

endif

endif

Cheers!

Andrej

BishopWolf commented 9 years ago

Ok I will use it, but since is CLHEP related I think it will be overwritten if Gate changes CLHEP version, which happens commonly. There is also an additional issue here, in CMAKEFILES there must be a switch to enable c++11 if detected Geant4 compiled with this, is there an specific def that I must rely on?

astuden100 commented 9 years ago

Maybe I can suggest that it probably will not be Geant, but ROOT that dictates the usage of c++11, which is in use from 6.0 onwards. So I guess checking for root-config --version might do the trick ...

Andrej

On 05/27/15 14:23, Alex Vergara Gil wrote:

Ok I will use it, but since is CLHEP related I think it will be overwritten if Gate changes CLHEP version, which happens commonly. There is also an additional issue here, in CMAKEFILES there must be a switch to enable c++11 if detected Geant4 compiled with this, is there an specific def that I must rely on?

— Reply to this email directly or view it on GitHub https://github.com/OpenGATE/Gate/issues/22#issuecomment-105888397.

BishopWolf commented 9 years ago

I added an option to cmakelists to quickly and easily enable c++11 standard when compiling, it is already published on https://github.com/OpenGATE/Gate/pull/21

dsarrut commented 9 years ago

Hello both of you,

ok to make Gate compatible with c++11. To my understanding only one simple modification is required (std:: before some ifstream),. CXX flag should be left to the user yet (not c++11 by default). The CLHEP issue : we will have to update clhep embedded in Gate source. Am I right ? If so, could you please provide a pull-request with the very minimal modification to make Gate compatible to c++11 (ofstream things + cmake option from Alex).

Alex: I cannot accept your pull request #21. There are too many different changes, each of them would require careful testing. I cannot accept change (iterator, ++i) that do not solve a very explicit issue: the risk they change Gate behaviour is too high. Remember that there are a lot of applications (TEP, SPECT, RT, CT, etc) and it is really difficult to test all. To date, the tests are still manually run. There is a project of doing it automatically (see the dashboard http://my.cdash.org/index.php?project=GATE), but it is still in progress.

For accepting contributions : 1) provide pull-request with a minimal change only focus on a single issue 2) briefly describe the initial issue and the goal of the changes
3) run all benchmarks in the folder benchmarks, before and after the change, and compare results.

Sorry I don't want to dim your enthusiasm, but Gate is an "old" code used by too many people, so it is not easy to changes :)

BishopWolf commented 9 years ago

Dear David if I prove all benchmarks give the same results then will you accept the changes? There are really a hard work done over here, even with bug catching so not to bug Gate and so on.

There are other sane solution, I can provide a testing branch and you can pull it to the code, the enthusiast users can test it and if everything is fine for them then merge it to develop. Does this sound reasonable to you?

Regards

mojca commented 9 years ago

Dear Alex,

Please note that I'm speaking as someone who is not a Gate developer (and as someone who didn't try to understand your changes either).

Not just the code changes, but the pull request overall is very hard to follow: "bug fixes", "merge", "yet another bugfix", "cleaning", "yet another fix", ... This is a reasonable personal workflow, but it would really simplify things for the developers if you could split the changes in smaller, but consistent and self-containing pull requests.

An example would be to create:

BishopWolf commented 9 years ago

Dear Mojca

As I already said, there can be a testing branch on Gate so this great pull requests can be merged without comprise Gate develop branch, users can test it (run benchmarks or personal examples) and report if the changes work or not (this case is even more important so not to work in vane).

My report is that with all those changes I can still run all benchmarks and I obtain good results from personal macros that touch different parts of the code (voxelized sources and geometries, gps sources, ion sources, "normal" geometries and so on).

Once you obtain some testing reports from enough people then you can safely merge it to develop. Regards Alex

BishopWolf commented 9 years ago

Done in https://github.com/OpenGATE/Gate/pull/23

BishopWolf commented 9 years ago

As the solution for this issue is already merged I recommend to close the issue

dsarrut commented 9 years ago

was not correct : change GATE_USE_STDC11 in the CMakeLists.txt SHA: 898daf7ba6508ba921eac310f27949ee783534be