mrzv / reeber

a library for shared- and distributed-memory parallel computation of merge trees
Other
7 stars 5 forks source link

Add explicit initializer types to compile with C++11 #1

Closed klacansky closed 4 years ago

klacansky commented 4 years ago

Not sure if this is acceptable, but it may be worthwhile to require C++11 instead of C++14. I would appreciate discussion, especially if this impacts future goals of reeber. No need to feel any pressure, I can easily maintain fork with the minor changes.

mrzv commented 4 years ago

I'm conflicted on this. On the one hand, I'm eager to move to C++14. It's ubiquitous enough, and the advantages are worthwhile. On the other hand, your changes are small and benign (except I need to figure out what broke in CI).

@grey-narn What's your opinion on this?

I should also note that VerticesIterator is slow. It's a vestige of an old reeber approach. diy::for_each() is a better way to do this, and one of the opportunities to speed up reeber.

anigmetov commented 4 years ago

If I remember correctly, the real reason for having C++14 is masked_box for AMR data, where 2 or 3 functions from the range library are combined, which makes the true type rather complicated; decltype(auto) works pretty well there. Would it be good to specify the required standard not for the whole project, but for each executable? I mean, I wouldn't mind having the rest of Reeber on C++11, but keep the AMR part on C++14 (AMReX codes use this standard, too).

On Fri, 28 Aug 2020 at 09:52, Dmitriy notifications@github.com wrote:

I'm conflicted on this. On the one hand, I'm eager to move to C++14. It's ubiquitous enough, and the advantages are worthwhile. On the other hand, your changes are small and benign (except I need to figure out what broke in CI).

@grey-narn https://github.com/grey-narn What's your opinion on this?

I should also note that VerticesIterator is slow. It's a vestige of an old reeber approach. diy::for_each() is a better way to do this, and one of the opportunities to speed up reeber.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mrzv/reeber/pull/1#issuecomment-682882062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALP2ZG76BFNECEGKXKVEP3SC7ODPANCNFSM4QOKDFRQ .

-- Best regards, Arnur Nigmetov

mrzv commented 4 years ago

Right, the main advantage of C++14 is that in C++11 you can't return lambdas from functions, which makes ranges much less usable. I'm not all that keen on having different requirements for different parts of the library, but maybe it's a reasonable compromise.

@pavolklacansky Does this pull request really fix all the C++14 vs C++11 issues (outside of AMReX, which I guess you are not using), or are there more issues?

mrzv commented 4 years ago

I'm just going to merge this, so I don't forget about it. The larger question of C++11 vs C++14 remains.

klacansky commented 4 years ago

I agree that adding multiple versions of C++ as a requirement adds extra complexity, and I would not be in favor of that. The package can officially require C++14 across the board (supported by maintainers) while still compiling as much as possible under C++11. I must admit that I am bit biased here and tend to lag decade or two behind the latest standard. Thank you for the discussion.

These are the cmake options I compiled with successfully on C++11 standard:


 CMAKE_BUILD_TYPE                 Release
 CMAKE_INSTALL_PREFIX             /home/sci/klacansky/reeber/install
 DIY_INCLUDE_DIR                  /home/sci/klacansky/diy/install/include
 Eigen3_DIR                       Eigen3_DIR-NOTFOUND
 HDF5_C_LIBRARY_dl                /usr/lib64/libdl.so
 HDF5_C_LIBRARY_hdf5              /usr/lib64/libhdf5.so
 HDF5_C_LIBRARY_m                 /usr/lib64/libm.so
 HDF5_C_LIBRARY_sz                /usr/lib64/libsz.so
 HDF5_C_LIBRARY_z                 /usr/lib64/libz.so
 HIGHFIVE_EXAMPLES                ON
 HIGHFIVE_PARALLEL_HDF5           OFF
 HIGHFIVE_UNIT_TESTS              ON
 HIGHFIVE_USE_BOOST               ON
 HIGHFIVE_USE_EIGEN               OFF
 HIGHFIVE_USE_XTENSOR             OFF
 amrex                            OFF
 counters                         ON
 persistent-integral-trace-vert   ON
 profile                          ON
 slow-tests                       ON
 trace                            ON
 use-tbb                          OFF
 xtensor_DIR                      xtensor_DIR-NOTFOUND```