[Tree] Emit error before crashing when reading type w/ no dictionary#21349
[Tree] Emit error before crashing when reading type w/ no dictionary#21349dpiparo wants to merge 1 commit intoroot-project:masterfrom
Conversation
Test Results 20 files 20 suites 2d 18h 5m 47s ⏱️ For more details on these failures, see this check. Results for commit fd476a3. ♻️ This comment has been updated with latest results. |
| } | ||
| } // namespace cling | ||
|
|
||
| namespace ROOT::Internal::TTreeReaderHelpers { |
There was a problem hiding this comment.
Indeed, this is a remnant of a refactoring. Sorry about that.
as TTree would do. Fixes root-project#10254
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
I am probably missing the bigger picture here, I left a few comments, some are corrections and some are for discussion
| const char *writeStlWithoutProxyMsg = | ||
| "The class requested (%s) for the branch \"%s\" " | ||
| "is an instance of an stl collection and does not have a compiled CollectionProxy. " | ||
| "Please generate the dictionary for this collection (%s) to avoid to write corrupted data."; |
There was a problem hiding this comment.
| "Please generate the dictionary for this collection (%s) to avoid to write corrupted data."; | |
| "Please generate the dictionary for this collection (%s) to avoid writing corrupted data."; |
| "The class requested (%s) for the branch \"%s\" " | ||
| "is an instance of an stl collection and does not have a compiled CollectionProxy. " | ||
| "Please generate the dictionary for this collection (%s) to avoid to write corrupted data."; | ||
| // This error message is repeated several times in the code. We write it once. |
There was a problem hiding this comment.
What is this comment about? The error message will be printed once per function call, isn't it?
There was a problem hiding this comment.
correct. I need to address that.
There was a problem hiding this comment.
Reading again this code and the code in TTree, I think I understand that the comment merely mentions that the same string needs to be used in multiple different functions, so instead of repeating it multiple times in the code, it is "written once" in this function. The error will still be printed once each time this situation is triggered, as it should.
| TEST(TTreeReaderBasic, WarnNoDict) | ||
| { | ||
| using rareType = std::map<std::string, bool>; | ||
| auto c = TClass::GetClass(typeid(rareType)); |
There was a problem hiding this comment.
We need to check for validity of the TClass * here
There was a problem hiding this comment.
TClass will always return for templates, because of automatic instantiation. We can decide to be on the safe side though, and even put an error to detect the ill-defined case where that does not happen.
There was a problem hiding this comment.
Shouldn't it be then GetClass<rareType>?
| // Create the input file in a separate process. | ||
| // This is necessary because we want to create a dictionary but not have it at disposal | ||
| // for the read |
There was a problem hiding this comment.
In other tests we treat this situation with a separate executable that creates the dictionary, then use ROOTTEST_GENERATE_DICTIONARY with the proper FIXTURES_SETUP to create a build dependency. Ideally we would do the same here, although I wouldn't put this as a blocker for this PR.
3 participants
as TTree would do.
Fixes #10254