Repository navigation
[ntuple] Throw informative error when collection proxy dictionary is missing (#21274) - #23766
ishaaaan17 wants to merge 1 commit into
Conversation
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
In principle looks good to me.
| // Proxied collections require a compiled dictionary for C++ iterators and element types. | ||
| // Eagerly check here to avoid low-level assertion failures or segfaults during proxy inspection (issue #21274). | ||
| if (!classp->HasDictionary()) | ||
| throw RException(R__FAIL(std::string(classp->GetName()) + " has no dictionary entry available")); | ||
| if (!classp->GetCollectionProxy()) | ||
| throw RException(R__FAIL(std::string(classp->GetName()) + " has no associated collection proxy")); |
There was a problem hiding this comment.
Swap these two conditions.
There was a problem hiding this comment.
done @jblomer! i swapped the two checks, so GetCollectionProxy() is checked first and then HasDictionary(). thanks!
16d0d8a to
ac5db1b
Compare
| // Eagerly check here to avoid low-level assertion failures or segfaults during proxy inspection (issue #21274). | ||
| if (!classp->GetCollectionProxy()) | ||
| throw RException(R__FAIL(std::string(classp->GetName()) + " has no associated collection proxy")); | ||
| if (!classp->HasDictionary()) |
There was a problem hiding this comment.
This condition in itself might be too strict. In particular for the case of std::pair, we only need to have its 2 argument to have a dictionary (or be a pair).
Related, if emulated collection proxy are supported here, the underlying line type would not have a dictionary.
There was a problem hiding this comment.
Correcting myself, the test is not on the containee but on the container. It has the side effect (intentional or not) of prevent the use of emulated collection proxy.
There was a problem hiding this comment.
thanks for the note @pcanal!
yes, it is intentional that RProxiedCollectionField does not allow emulated collection proxies. RProxiedCollectionField depends on compiled iterator function pointers (proxy->GetFunctionCreateIterators(), etc., in RCollectionIterableOnce::GetIteratorFuncs, line 1322), and these are not available for emulated collection proxies.
for on-disk collections that have no compiled dictionary, RNTuple reads them as untyped emulated vector fields (CreateEmulatedVectorField) and not through RProxiedCollectionField.
This branch has not been deployed
4 participants
summary of changes
fixes #21274.
when an RNTuple collection proxy field (for example
std::setorstd::map) was created for a type that has no dictionary, ROOT hit an assertion failure inTGenCollectionProxy.cxx:401(R__ASSERT(e->IsValid())), or failed while inspecting the collection proxy.this change:
classp->HasDictionary()in the constructor ofRProxiedCollectionField, before the collection proxy is generated, and throws aROOT::RExceptionwith the name of the missing type ("<type> has no dictionary entry available").tree/ntuple/test/ntuple_types.cxxunderTEST(RNTuple, TVirtualCollectionProxy). it checks that a collection type without a loaded dictionary (std::map<double, double>) throws aROOT::RExceptionwith this error message.checklist
.clang-formatai disclosure
Assisted-by: antigravity:gemini. i used an ai assistant to follow the collection proxy generation call stack in
RProxiedCollectionFieldandTGenCollectionProxy. i checked the dictionary lookup againstTClass::HasDictionary(), wrote the unit test and the exception check, and checked the formatting with.clang-format.