[ntuple] Remove type name from public field constructors #19875
Conversation
jblomer
force-pushed
the
ntuple-fix-field-constructors
branch
from
cb3a737 to
4d80ea9
Compare
including ROptionalField and RUniquePtrField
| std::unique_ptr<RFieldBase> itemField) | ||
| : ROOT::RFieldBase(fieldName, typeName, ROOT::ENTupleStructure::kCollection, false /* isSimple */) | ||
| : ROOT::RFieldBase(fieldName, typePrefix + "<" + itemField->GetTypeName() + ">", ROOT::ENTupleStructure::kCollection, |
There was a problem hiding this comment.
As implied by:
This is dangerous as it potentially allows to write arbitrary type names to disk.
doesn't the 'prefix' then also need to be normalized? (At least here) wasn't the 'real' issue that the 2 types (typeName and the one derived from itemField->GetTypeName() being potentially different?
There was a problem hiding this comment.
No, because the constructor that accepts a prefix is protected. I could have used the full type name instead of the type prefix approach. The reason I didn't is that the code calling this constructor would then both get the type name from the item field and move the item field to the base class. And doing both these things in a single call is illegal.
There was a problem hiding this comment.
Wasn't the previous constructor also protected?
There was a problem hiding this comment.
Yes, the problem was not with RNullableField but with its descendants ROptionalField and RUniquePtrField that do have a public constructor.
There was a problem hiding this comment.
Can this be resolved?
Test Results 20 files 20 suites 3d 14h 48m 57s ⏱️ For more details on these failures, see this check. Results for commit 4d80ea9. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
In principle LGTM. It smells a bit that we have to repeat the field type names in a number of places and sometimes passed through TClass, but I don't have a better proposal for the moment. See also question by Philippe regarding the constructor of RNullableField.
Several RFieldBase descendants still allow for passing the type name in a public constructor. This is dangerous as it potentially allows to write arbitrary type names to disk. Remove / rework these constructors.