Status bits rationalization .... #917

pcanal · 2017-08-31T20:17:51Z

This introduces TStatusBitsChecker which allows to check whether there is 'Status bits' overlap in a given class hierarchy.

Expectedly, there are several overlaps ....

This merge request also annotate (by properly naming the enum type as EStatusBits) as needed and resolves the reported errors in core, io/io, tree/tree and hist/hist.

Some overlaps can not be resolve to preserve forward compatibility and after analysis of which bit is overlapping should be 'harmless' and thus are marked to be ignored by the checker tools (by adding a enum type named EStatusBitsDupExceptions).

Usage example:

return TStatusBitsChecker::CheckAlClasses();

// or without EStatusBitsDupExceptions in TStreamerElement.
TStatusBitsChecker::Check("TStreamerElement");

Error in <TStatusBitsChecker>: In TStreamerElement class hierarchy, there are duplicates bits:
Error in <TStatusBitsChecker>:    Bit   6 used in TStreamerElement as kHasRange
Error in <TStatusBitsChecker>:    Bit   6 used in TObject as kCannotPick
Error in <TStatusBitsChecker>:    Bit  13 used in TStreamerElement as kDoNotDelete
Error in <TStatusBitsChecker>:    Bit  13 used in TObject as kInvalidObject

phsft-bot · 2017-08-31T20:18:04Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-08-31T21:06:21Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-08-31T21:08:02Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-08-31T21:16:51Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-08-31T21:24:32Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-08-31T21:40:50Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

Axel-Naumann

Only tiny comments - plus I don't think this really addresses the problem but any alternative is too expensive... LGTM after addressing the test failure.

Axel-Naumann · 2017-09-01T04:12:16Z

core/meta/inc/TClass.h

+   enum EStatusBits {
+      kClassSaved  = BIT(12),
+      kIgnoreTObjectStreamer = BIT(15),
+      kUnloaded    = BIT(16), // The library containing the dictionary for this class was


As you're touching this anyway, can you use pre-member doxygen doc like so:

/// The library containing the dictionary for this class was /// loaded and has been unloaded from memory. kUnloaded = BIT(16),

This makes it easier to read and write precise documentation and enables doxygen to actually pick it up.

The annoying part is that (visually at the very least), unless there is a empy line above the comment, I never 'know' whether the comment applies to the one before or after.

But we're paid by lines of code, so adding empty lines is a wonderful thing! ;-)

Axel-Naumann · 2017-09-01T04:12:55Z

core/meta/inc/TClass.h

+      kStartWithTObject = BIT(20),  // see comments for IsStartingWithTObject()
+      kWarned      = BIT(21),
+      kHasNameMapNode = BIT(22),
+      kHasCustomStreamerMember = BIT(23) // The class has a Streamer method and it is implemented by the user or an older (not StreamerInfo based) automatic streamer.


Axel-Naumann · 2017-09-01T04:19:18Z

tree/tree/inc/TBranchObject.h

+      kWarn = BIT(14)
+   };
+
+   enum EStatusBitsOldValues {


Can you document why you have this?

Axel-Naumann · 2017-09-01T04:19:40Z

tree/tree/inc/TLeafObject.h

+      kWarn = BIT(14)
+   };
+
+   enum EStatusBitsOldValues {


Axel-Naumann · 2017-09-01T04:22:08Z

core/meta/src/TStatusBitsChecker.cxx

+#include "TStatusBitsChecker.h"
+
+#include "TClass.h"
+#include "TBaseClass.h"


The order is "almost-alphabetical" ;-)

Axel-Naumann · 2017-09-01T04:26:47Z

core/meta/src/TStatusBitsChecker.cxx

+
+void TStatusBitsChecker::Registry::RegisterBits(TClass &classRef /* = false */)
+{
+   TEnum *eStatusBits = (TEnum*)classRef.GetListOfEnums()->FindObject("EStatusBits");


What if the name is different (as in many examples in ROOT before your patch)?
Isn't the relevant criterium that SetBit() is called on such an enum constant? That would mean we need an external tool looking at invocations...

Isn't the relevant criterium that SetBit() is called on such an enum constant? That would
mean we need an external tool looking at invocations...

Yes and it would technically have to parse the entire world's sources to find out. Doing so with the ROOT source would be a good start though :) .... Anyway, this way is 'much' cheaper (for me) to write (it would also have to deal with indirect setting and combinations SetBit(aliasOne | aliasTwo))

As importantly, having this enum clearly name helps with documentation ... (i.e if one was to write the parsing tool, it could complain that a bit is not listed in EStatusBits)

Lastly writing the EStatusBitsDupExceptions or equivalent would still be necessary in some cases ...

Axel-Naumann · 2017-09-01T04:28:11Z

core/meta/src/TStatusBitsChecker.cxx

+   if (eStatusBits) {
+
+      for(auto c : *eStatusBits->GetConstants()) {
+         TEnumConstant *constant = dynamic_cast<TEnumConstant*>(c);


Coverity will be unhappy with you :-) What if the dyn_cast fails?

It can't :) the 'original' version of the code was

TEnumConstant *constant = (TEnumConstant*)(c);

Axel-Naumann · 2017-09-01T04:30:16Z

core/meta/src/TStatusBitsChecker.cxx

+
+}
+
+// Return false if there is any unexpected duplicates BIT constant in


Why isn't this a proper doxygen comment?

What does doxygen do with this lame comment?

humm ... actually what is our current strategy ... the official documentation is in the header files. Should we leave the source file completely undocumented?

ROOT's function doc is in the sources.

Axel-Naumann · 2017-09-01T04:30:50Z

core/meta/src/TStatusBitsChecker.cxx

+   return Registry().Check(classRef,verbose);
+}
+
+// Return false if there is any unexpected duplicates BIT constant in


Axel-Naumann · 2017-09-01T04:31:11Z

core/meta/src/TStatusBitsChecker.cxx

+{
+   bool result = true;
+
+   // start from begining


vgvassilev · 2017-09-01T08:39:30Z

Could you add a gtest for this new feature?

phsft-bot · 2017-09-01T15:13:02Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-01T15:16:14Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-01T16:06:14Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T16:12:30Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T16:14:37Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T16:21:31Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T16:23:05Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

This allows for automatic checking overlaps in a given class hierarchy.

The value conflicted with TBranch::kBranchObject.

The value conflicted with TLeaf::kNewValue.

It was conflicting with TApplication::kDefaultApplication. This are used transiently so no backward compatibility issues

…hough they should have been)

It was conflicting with TObject::kCanDelete. Even-though it is persistent information, it is only ever used when first creating the branch and thus changing the value should have not practical down-side.

…reamerInfo This specially named Enum will allow to annotate the EStatusBits enum to list the bit that are intentionally/knowingly duplicated other bits in the clas hierarchy

TStatusBitsChecker::Check and TStatusBitsChecker::CheckAllClasses will determine if the set of "status bit" declared in the class and its base classes presents any overlap. The status bit are declared in a given class by declaring an enum type named EStatusBits. If some of the duplication is intentional, those duplication can be registered in an enum type named EStatusBitsDupExceptions. ~~~ {.cpp} // TStreamerElement status bits enum EStatusBits { kHasRange = BIT(6), kCache = BIT(9), kRepeat = BIT(10), kRead = BIT(11), kWrite = BIT(12), kDoNotDelete = BIT(13), kWholeObject = BIT(14) }; enum class EStatusBitsDupExceptions { // This bit duplicates TObject::kInvalidObject. As the semantic of kDoNotDelete is a persistent, // we can not change its value without breaking forward compatibility. // Furthermore, TObject::kInvalidObject and its semantic is not (and should not be) // used in TStreamerElement kDoNotDelete = TStreamerElement::kDoNotDelete, // This bit duplicates TObject::kCannotPick. As the semantic of kHasRange is a persistent, // we can not change its value without breaking forward compatibility. // Furthermore, TObject::kCannotPick and its semantic is not (and should not be) // used in TStreamerElement kHasRange = TStreamerElement::kHasRange }; ~~~ {.cpp} Without the EStatusBitsDupExceptions enum you would see ~~~ {.cpp} TStatusBitsChecker::Check("TStreamerElement"); Error in <TStatusBitsChecker>: In TStreamerElement class hierarchy, there are duplicates bits: Error in <TStatusBitsChecker>: Bit 6 used in TStreamerElement as kHasRange Error in <TStatusBitsChecker>: Bit 6 used in TObject as kCannotPick Error in <TStatusBitsChecker>: Bit 13 used in TStreamerElement as kDoNotDelete Error in <TStatusBitsChecker>: Bit 13 used in TObject as kInvalidObject ~~~ {.cpp}

frexp ultimately give the same information (what power of 2 is this number) but is 3 times faster than the pair (log2, nearbyint).

…ubmerger. Error in <TStatusBitsChecker>: In TProofPlayer class hierarchy, there are duplicates bits: Error in <TStatusBitsChecker>: Bit 16 used in TProofPlayer as kIsProcessing Error in <TStatusBitsChecker>: Bit 16 used in TVirtualProofPlayer as kIsSubmerger Error in <TStatusBitsChecker>: In TProofServ class hierarchy, there are duplicates bits: Error in <TStatusBitsChecker>: Bit 16 used in TProofServ as kHighMemory Error in <TStatusBitsChecker>: Bit 16 used in TApplication as kDefaultApplication

phsft-bot · 2017-09-01T21:31:31Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-01T22:07:10Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-01T22:50:07Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T22:57:12Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T22:57:58Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T22:59:25Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

phsft-bot · 2017-09-01T23:24:19Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments_auto

pcanal requested review from Axel-Naumann, couet, gganis and lmoneta as code owners August 31, 2017 20:17

Axel-Naumann approved these changes Sep 1, 2017

View reviewed changes

pcanal force-pushed the StatusBits branch from c19d3c2 to 22a0f7b Compare September 1, 2017 15:12

pcanal force-pushed the StatusBits branch from 22a0f7b to 5d537e9 Compare September 1, 2017 15:16

pcanal added 9 commits September 1, 2017 12:06

Remove never used enum (TRef::kNotComputed)

67d2d49

Remove enum TClonesArray::kNoSplit not used since 2.26 (circa 2001)

f7b63a2

Update release notes

e9c82a3

Mark status bits as 'enum EStatusBits' in core, i/o, tree and hist.

9a9d54f

This allows for automatic checking overlaps in a given class hierarchy.

Remove never used enum constant TLink::kObjIsParent

4b8053e

Change value of TBranchObject::kWarn. Reset it upon reading.

18c9cb5

The value conflicted with TBranch::kBranchObject.

Change value of TLeafObject::kWarn. Reset it upon reading.

8084146

The value conflicted with TLeaf::kNewValue.

Change value of TApplicationRemote::kCollecting.

20d96bb

It was conflicting with TApplication::kDefaultApplication. This are used transiently so no backward compatibility issues

Remove information duplication

1bc4ff7

pcanal added 10 commits September 1, 2017 12:06

Mark TObject::kObjInCanvas as deprecated

87a10fc

Add comments on status bits that are not available to TObject (even-t…

71f2c55

…hough they should have been)

Update value of TClonesArray::kForgetBits.

e2bf336

It was conflicting with TObject::kCanDelete. Even-though it is persistent information, it is only ever used when first creating the branch and thus changing the value should have not practical down-side.

Introduce EStatusBitsDupExceptions in TStreamerElement and TVirtualSt…

67609c6

…reamerInfo This specially named Enum will allow to annotate the EStatusBits enum to list the bit that are intentionally/knowingly duplicated other bits in the clas hierarchy

In ConvertToBit replace log2,nearbyint with std::frexp.

127c729

frexp ultimately give the same information (what power of 2 is this number) but is 3 times faster than the pair (log2, nearbyint).

White spaces [NFC]

ae8a0ce

Typo [NFC]

f2da26f

Add TSystem::LoadAllLibs

3f043a6

pcanal force-pushed the StatusBits branch from 5d537e9 to 28b43f6 Compare September 1, 2017 21:31

Add TStatusBitsChecker.cxx gtest

3fa653f

pcanal requested a review from peremato as a code owner September 1, 2017 22:06

pcanal merged commit 5c8c3e8 into root-project:master Sep 1, 2017

pcanal deleted the StatusBits branch September 3, 2017 03:51

phsft-bot mentioned this pull request Oct 2, 2018

[cxxmodule] Don't declare fwd decls from rootmaps with modules #2592

Merged

phsft-bot mentioned this pull request Apr 20, 2022

[RF] Remove deprecated version of createHistogram to see what happens #10419

Closed

phsft-bot mentioned this pull request Jul 5, 2023

[RF] Add methods to create owning RooFit proxies via std::unique_ptr #12924

Merged


		}

		// Return false if there is any unexpected duplicates BIT constant in

Status bits rationalization .... #917

Status bits rationalization .... #917

Uh oh!

Conversation

pcanal commented Aug 31, 2017

Uh oh!

phsft-bot commented Aug 31, 2017

Uh oh!

phsft-bot commented Aug 31, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 31, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 31, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 31, 2017

Failing tests:

Uh oh!

phsft-bot commented Aug 31, 2017

Failing tests:

Uh oh!

Axel-Naumann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented Sep 1, 2017

Uh oh!

phsft-bot commented Sep 1, 2017

Uh oh!

phsft-bot commented Sep 1, 2017

Uh oh!

phsft-bot commented Sep 1, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 1, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 1, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 1, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 1, 2017

Axel-Naumann left a comment •

edited

Loading