iLCSoft / Marlin

Modular Analysis and Reconstruction for the LINear Collider
GNU General Public License v3.0
11 stars 16 forks source link

Fix warnings #8

Closed andresailer closed 7 years ago

gaede commented 7 years ago

Why do you use a global variable here: https://github.com/iLCSoft/Marlin/pull/8/commits/12e00ec064e14db0ac831050bca756a2202286f0#diff-6c43096941a8c63137f1b9c8718d0c0bR9

gaede commented 7 years ago

On could argue about that. In any case changing only one out of ten #defines to a const int seems awkward to me as it confuses readers of the code. Can you please change back to #define ( or consistently replace all defines )

andresailer commented 7 years ago

No I really don't think you can argue about the fact that constexpr is better than define. But I will revert because the define was not related to any warning.

gaede commented 7 years ago

Yes one can argue about that in certain use cases. For example would your suggested solution:

constexpr int MAX_ERRORS=3;

have introduced a global variable in the global namespace and prevented inclusion of the header file by more than one *.cc file in the compilation unit. Admittedly not a problem in this particular case, but an example where a #define would have been the better choice ( though not necessarily the best, of course).

Anyways, thanks for fixing the warnings - I'll merge the PR.

andresailer commented 7 years ago

A *.cc file is a compilation unit, unless you are doing weird things(i.e. including another cc file). One can effectively only include the header file once in a compilation unit, because of the header guards. And even including the header in an additional processor doesn't break the compilation. Now using non-const variables would cause linker errors if the header file is included more than once.

gaede commented 7 years ago

I meant of course linking-unit - and it does not make it better that it compiles but crashes the linking step...

andresailer commented 7 years ago

But it links with const or constexpr.

gaede commented 7 years ago

oops, you are right of course - seems I got this confused with C...

andresailer commented 7 years ago

BEGINRELEASENOTES

ENDRELEASENOTES