Code conventions and C++ standards in ROOT

[behrenhoff]

Hi there,

sorry for flooding the forum with questions today. Before modifying ROOT code, I’d like to know:

a) what C++ standard may I use when modifying code (in TMVA)? C++11 is certainly OK, I assume? What about C++14, i.e. can I use std::make_unique? On the other hand, TDataFrame is already using std::string_view, i.e. C++17. Can I use any C++17 feature?! (how can it be that ROOT can still be compiled with a pre C++17 compiler?!)

b) What about the the ifdefs with #if __cplusplus > 199711L - is the pre C++11 code still necessary or can it be removed?

c) How to check pointers for nullptr? Do you prefer
c1) if (p) / if (!p)
or
c2) if (p != nullptr) / if (p == nullptr)?
c1) is suggested in the C++ Core Guidelines ES.87: Don’t add redundant == or != to conditions, while c2) is suggested by the readability-implicit-bool-conversion clang-tidy checker - I have seen both styles in the code.

[eguiraud]

Hi,
@Axel or @pcanal might correct me if I’m wrong, but afaik:

a) C++11 plus a few selected things that ROOT backported to C++11 from subsequent standards: make_unique is in ROOT/RMakeUnique.hxx, string_view is in RStringView.hxx (soon-to-be ROOT/RStringView.hxx),std::spanwill soon be inROOT/RSpan.hxx` (see also this issue)

b) the pre-C++11 code is not necessary. ROOT6 requires C++11

c) I’m pretty sure we all agree on if (p == nullptr), but since it’s not written in the coding conventions I might be mistaken

As for code formatting and naming, consistency with the surrounding lines comes first, otherwise clang-format is the authority (our configuration file is in the coding conventions page).

[behrenhoff]

Ah, and I thought adding things to namespace std was undefined behavior

Preferring c2: very interesting. I prefer c1 most of the time, I add “redundant” ==/!=0 only when using strcmp.

And I’m using clang-format all the time, the config file is even in the code repository, even though ColumnLimit 120 seems rather large to me…

[Axel]

Hi,

Simpler: we backport a couple of things and they will end up in ROOT/span.hxx and ROOT/string_view.hxx and ROOT/memory.hxx (make_unique). If you’re in dire need of a C++14/17 feature we will consider adding it.

We usually use if (!p) as that’s a simpler read. optional etc also work nicely with this pattern; == nullptr does not.

@behrenhoff

Ah, and I thought adding things to namespace std was undefined behavior

Indeed, but we’re wiring code for real not for the standard, so undef behavior isn’t killing us here. Not at all.

I’ll push more detailed coding conventions into root.git based on examples “soon”.

Cheers, Axel.

[behrenhoff]

Actually optional, make_unique, and string_view (and the Str* functions) were the reasons for me to include abseil as dependency in my code, i.e. using absl::make_unique and absl::optional. I even had my own version of make_unique before abseil was released. I didn’t know I could have simply included corresponding ROOT headers. Amazing.

By the way, thanks for your work on reflection (https://youtu.be/4AfRAVcThyA?t=698)!

[Axel]

Hi @behrenhoff - I disagree with a lot of the underlying assumptions of abseil, at least for our context (HEP with many casual coders). It might work just fine for big companies that can fire people who don’t code along the company lines.

And you’re welcome for the reflection work!