[io] Make case values constant expressions #11148
Conversation
|
Starting build on |
|
A tool (Coverity) was complaining that the values used in the switch statement did not belong to the enum. (See ac36d87). I am guessing to avoid this we might also want to apply something like: |
Unless I'm looking at the wrong data structure, root/io/io/inc/TGenCollectionProxy.h Line 64 in cd99254 I don't think casting will change anything here. |
|
I don't know if I'm misinterpreting something, but I noticed something very interesting: Is this enum type That may said, the compiler got confused about these two types that have same name? |
No, it uses (sparsely) the full range of a 32 bis integer (by using it essentially as a bitfield). |
You a right. I am guessing that Coverity noticed that some of the case value were EProperty and complained that some weren't (since there operator |
Humm ... either I don't understand the meaning or Clang is 'wrong' on this part. Both are "constant" (i.e calculatable at compile time).
That is likely the cause of the complaints albeit it is the developer (and possibly Coverity too) that got confused here as it was meant to be written as: |
I can confirm this fixes the error, using the diff below: However, then another error occured: |
That's interesting:
As discussed post-commit in https://reviews.llvm.org/D130058, the check is indeed questionable in a few corner cases. But that shouldn't stop us from finding a portable solution that avoids the problems altogether.
The range checks of that new warning are also known bad for some cases, not sure if that is one of them...
I still find it interesting that this works. However, as said before, the type of |
This is actually a different, unrelated problem. Should be fixed by #11152 |
In this case it ended being relevant as indeed the wrong enum type was being used and it could (theoretically) have lead to slicing the value.
However, in this case, they are "semantically" part of the enums values.
That did not prevent Coverity from complaining that some of the case value where from an enum and some where numerical value (that it assumed were unrelated). |
Doesn't matter what we semantically want them to be. For the compiler they aren't, so it's undefined behavior (at least that's how I understand the entire discussion).
I don't care what Coverity thinks or complains about. I'm arguing that having integer constants is more correct than what we do right now, and it seems you don't disagree with this point, do you? |
Fair enough. Can you update the git log to reflect that the 'reversion' (instead of tweak) of the previous commit is intentional (and needed). |
Current clang-16 from main complains: "case value is not a constant expression". Even if that error is probably relaxed before Clang 16 is released early next year, there is really no point in converting an integer into an enum just to get a numeric value back. This is (intentionally) a partial revert of commit ac36d87. An alternative solution appears to be replacing EProperty (which refers to the enum type in TVirtualCollectionProxy) with ::EProperty from TDictionary.h. However, since the enum values are used as a bit mask and fCase is defined as UInt_t anyhow, it makes more sense to compare integer constants from the start. Closes root-project#11128
hahnjo
force-pushed
the
io-case-clang16
branch
from
bd63998 to
b4bd659
Compare
|
Starting build on |
Done. I've also mentioned the ambiguity between |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
junaire
left a comment
junaire
left a comment
There was a problem hiding this comment.
Thanks, that looks good but maybe we can apply clang-format?
If I run diff --git a/io/io/src/TGenCollectionStreamer.cxx b/io/io/src/TGenCollectionStreamer.cxx
index a2acf0b8d0..9979884dae 100644
--- a/io/io/src/TGenCollectionStreamer.cxx
+++ b/io/io/src/TGenCollectionStreamer.cxx
@@ -389,14 +389,10 @@ void TGenCollectionStreamer::ReadObjects(int nElements, TBuffer &b, const TClass
switch (fVal->fCase) {
case kIsClass:
DOLOOP(b.StreamObject(i, fVal->fType, onFileValClass ));
- case kBIT_ISSTRING:
- DOLOOP(i->read_std_string(b));
- case kIsPointer | kIsClass:
- DOLOOP(i->set(b.ReadObjectAny(fVal->fType)));
- case kIsPointer | kBIT_ISSTRING:
- DOLOOP(i->read_std_string_pointer(b));
- case kIsPointer | kBIT_ISTSTRING | kIsClass:
- DOLOOP(i->read_tstring_pointer(vsn3, b));
+ case kBIT_ISSTRING: DOLOOP(i->read_std_string(b));
+ case kIsPointer | kIsClass: DOLOOP(i->set(b.ReadObjectAny(fVal->fType)));
+ case kIsPointer | kBIT_ISSTRING: DOLOOP(i->read_std_string_pointer(b));
+ case kIsPointer | kBIT_ISTSTRING | kIsClass: DOLOOP(i->read_tstring_pointer(vsn3, b));
}
#undef DOLOOP
break;
@@ -864,18 +860,10 @@ void TGenCollectionStreamer::ReadMap(int nElements, TBuffer &b, const TClass *on
case kIsClass:
b.StreamObject(i, v->fType);
break;
- case kBIT_ISSTRING:
- i->read_std_string(b);
- break;
- case kIsPointer | kIsClass:
- i->set(b.ReadObjectAny(v->fType));
- break;
- case kIsPointer | kBIT_ISSTRING:
- i->read_std_string_pointer(b);
- break;
- case kIsPointer | kBIT_ISTSTRING | kIsClass:
- i->read_tstring_pointer(vsn3, b);
- break;
+ case kBIT_ISSTRING: i->read_std_string(b); break;
+ case kIsPointer | kIsClass: i->set(b.ReadObjectAny(v->fType)); break;
+ case kIsPointer | kBIT_ISSTRING: i->read_std_string_pointer(b); break;
+ case kIsPointer | kBIT_ISTSTRING | kIsClass: i->read_tstring_pointer(vsn3, b); break;
}
v = fVal;
addr += fValOffset;which I find much worse to read... |
I agree ... let's keep the code formatted as is. |
Thanks. |
4 participants
Current
clang-16from main complains: "case value is not a constant expression". Even if that error is probably relaxed before Clang 16 is released early next year, there is really no point in converting an integer into an enum just to get a numeric value back.This is (intentionally) a partial revert of commit ac36d87. An alternative solution appears to be replacing
EProperty(which refersto the enum type in
TVirtualCollectionProxy) with::EPropertyfromTDictionary.h. However, since the enum values are used as a bit mask andfCaseis defined asUInt_tanyhow, it makes more sense to compare integer constants from the start.This PR fixes #11128