Made public TH1::CheckConsistency (2)

[amecca]

This Pull request:

This is a follow up of #13748, which was modified by
@guitargeek to fix the unnecessary use of exceptions.

Associated roottest PR:

• Update reference public members of TH1 roottest#1040

Changes or fixes:

Made the static function TH1::CheckConsistency public, and changed its return type to Int_t.
Since the meaning of the returned integer is actually part of EInconsistencyBits, I made the enum public as well (but left the return value type to Int_t). This is done in a separate commit in case it is decided to keep the enum itself as an implementation detail.

Motivation

At the moment TH1::CheckConsistency is called internally by TH1::Add, and error messages are print in case of failure, but continues the execution. Making it public allows user code to manually check the consistency to catch errors and stop the program. This is especially useful for programs that make hundreds of Adds, out of which only a few fail, and the error messages may be lost in logs and go unnoticed.

Checklist:

• tested changes locally
• updated the docs (if necessary)

[vepadulano]

If this is an interface that is being made public for the first time, I would take the chance to promote better code practices, e.g. by making the return type an enum class

[amecca]

If this is an interface that is being made public for the first time, I would take the chance to promote better code practices, e.g. by making the return type an enum class

I can do that, but all the other enums in the interface will remain plain C-style. Also, the return type will not be contextually convertible to bool anymore:

if(TH1::CheckConsistency(&h1, &h2)) {
   h1.Add(h2);
}

will not work.

If that's ok, I'll change the enum to enum class and the return type of the function.

Edit: also, all instances of code like

if( inconsistency & kDifferentDimensions )

will become a lot more verbose:

if( inconsistency & static_cast<UInt_t>(EInconsistencyBits::kDifferentDimensions) )

[guitargeek]

If this is an interface that is being made public for the first time, I would take the chance to promote better code practices, e.g. by making the return type an enum class

I can do that, but all the other enums in the interface will remain plain C-style. Also, the return type will not be contextually convertible to bool anymore

That's both valid. Indeed, when introducing this enum, I didn't make it an enum class mostly to be consistent with the other TH1 enums. The fact that it's less verbose is a nice extra here.

[github-actions]

Test Results

    18 files      18 suites   3d 22h 38m 53s ⏱️
 2 678 tests  2 610 ✅ 0 💤  68 ❌
46 490 runs  46 339 ✅ 0 💤 151 ❌

For more details on these failures, see this check.

Results for commit 840f3d8.

♻️ This comment has been updated with latest results.

[amecca]

I'm not sure if I am supposed to say this, but it seems that test that fails is checking for changes on the list of the public data members of TH1F [1], which is intended here.

[1] https://github.com/root-project/roottest/blob/e7c9a5cbc969fa99b668684cbebc59d554ceebc8/root/meta/runMemberComments.C#L34-L35
[2] https://github.com/root-project/roottest/blob/master/root/meta/Makefile#L140-L141

[pcanal]

@amecca This is correct. Can you propose a PR for the roottest repository that includes the required update to the test.

[amecca]

@pcanal, I opened root-project/roottest#1038

[pcanal]

In order for the roottest branch to be picked-up by the automated test you need to use the same branch name in both repository (so publicCheckConsistency2 in this case, it seems that on roottest you used publicTH1consistencyEnum

[amecca]

My bad, I renamed the branch and opened a new PR root-project/roottest#1040

[amecca]

It seems that either my roottest branch did not update correctly the reference file or it was not picked up by the automated test.

[guitargeek]

Thank you very much! The tests pass now.