Switch Carpet to new bboxset class implementation

Comments (12)

1. 

   Erik Schnetter reporter

   • changed status to open
   • removed comment

   • 2014-12-30T18:08:37+00:00
2. 

   Ian Hinder

   • removed comment

   I seem to remember that this new class required C++11. Is that the case? bboxset2.hh seems to have some workarounds in case lambdas are not available, so maybe this is no longer the case?

   I have glanced at the code, but do not have time to do a detailed code review, as there is a lot of code there. I assume that if it passes the current tests, it is appropriate to make this the default at this point in the release cycle, and if there are problems, that change can be reverted before the next release. If you feel this level of review is appropriate, please go ahead.

   One comment: this is the sort of code for which it is very easy to write unit tests. Assuming they don't already exist, where should they go? I have some local additions to the old bboxset code, and wanted to write tests for them, but wasn't sure of the best way of organising that.

   • 2015-01-13T07:00:17+00:00
3. 

   Erik Schnetter reporter

   • removed comment

   Yes, this requires C++11. There are very few systems where C++11 is not available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!).

   Yes, there are unit tests. They are in the thorn Carpet/TestBboxSet2.

   • 2015-01-13T07:20:08+00:00
4. 

   Ian Hinder

   • removed comment

   What sort of error message would people get at the moment if C++11 is not available, and they try to build the ET on a machine without the CARPET_DISABLE_BBOXSET2 macro defined? Should C++11 be detected automatically duing autoconf, and Carpet use this to decide whether the new code is used? Or should the check be more specific, for the features which are actually used?

   I am thinking of users trying to set up the ET on a new machine which does not have C++11 (are these sufficiently rare that we can assume users won't encounter them?) and getting an error message which does not clearly explain that they need to set a Carpet macro due to having an old compiler.

   Thanks for the unit tests; I actually looked for such a thorn, but for some reason didn't see it.

   • 2015-01-13T07:43:30+00:00
5. 

   Erik Schnetter reporter

   • removed comment

   The error message would likely (so I hope) complain about a C++11 feature not available, such as a range-based for loop or a lambda. If the compiler is really old, it may just be a syntax error. An autoconf macro would be nice.

   • 2015-01-13T08:47:21+00:00
6. 

   Ian Hinder

   • removed comment

   According to http://www.stroustrup.com/C++11FAQ.html#11, we could test the __cplusplus macro. See also http://stackoverflow.com/questions/5047971/how-do-i-check-for-c11-support.

   #if __cplusplus <= 199711L
     #error This library needs at least a C++11 compliant compiler
   #endif
   

   Maybe we could try this?

   • 2015-01-13T09:29:20+00:00
7. 

   Roland Haas

   • removed comment

   Replying to [comment:3 eschnett]:

   Yes, this requires C++11. There are very few systems where C++11 is not available, and there one needs to use a -DCARPET_DISABLE_BBOXSET2 to fall back to the previous implementation (hello, Zwicky!). Please have a look at simfactory2's current master and the zwicky-intel14 set of files in there.

   • 2015-01-14T05:04:27+00:00
8. 

   Frank Löffler

   • removed comment

   Replying to [comment:6 hinder]:

   According to http://www.stroustrup.com/C++11FAQ.html#11, we could test the __cplusplus macro. See also http://stackoverflow.com/questions/5047971/how-do-i-check-for-c11-support.

   #if __cplusplus <= 199711L #error This library needs at least a C++11 compliant compiler #endif

   Maybe we could try this?

   If such a way to test for C++11 is available, then we should not output an error in that case, but define CARPET_DISABLE_BBOXSET2 instead (in addition to a warning maybe), shouldn't we?

   • 2015-01-26T08:59:16+00:00
9. 

   Ian Hinder

   • removed comment

   I wasn't suggesting that code fragment literally; I was using it as a way to illustrate how C++11 can be checked for. On the other hand, is it confusing to have the version of the code used depend on the version of the compiler? There should probably at least be a warning at compile time such as "C++11 not found; falling back to old bboxset implementation", so that it is clear that the preferred version is the C++11 version, and using the old version is not recommended in some sense.

   • 2015-01-26T10:09:11+00:00
10. 

    Ian Hinder

    • removed comment

    [Sorry Frank, I didn't read your comment carefully enough, and missed that you had already mentioned a warning.]

    To push this ticket forward, let me make a concrete suggestion:

    If the user sets the CARPET_DISABLE_BBOXSET2 macro or `if __cplusplus <= 199711L`, then fall back to the old bboxset implementation and emit a compile-time warning that the old implementation is being used because C++11 is not available.
    

    This should mean that people don't have to explicitly configure for this feature on their machine. Then, if the tests are OK, as they apparently are, then I am happy for this to be made the default at this point in the release cycle.

    • 2015-01-26T10:52:44+00:00
11. 

    Erik Schnetter reporter

    • changed status to resolved
    • removed comment

    I implemented this not by checking the reported standard version, but by adding autoconf macros for the required features. I also added a build-time warning if Carpet disables the new bboxset class.

    • 2015-01-26T15:20:02+00:00
12. 

    Roland Haas

    • edited description
    • changed status to closed

    • 2019-02-21T20:00:59+00:00
13. Log in to comment