[rfile] Introduce ListKeys() method #20056
Conversation
silverweed
requested review from
enirolf,
hageboeck,
hahnjo,
jblomer and
vepadulano
Test Results 22 files 22 suites 3d 23h 9m 53s ⏱️ For more details on these failures, see this check. Results for commit ad5748c. ♻️ This comment has been updated with latest results. |
silverweed
force-pushed
the
rfile_02
branch
3 times, most recently
from
ed31e98 to
a001f6d
Compare
io/io/inc/ROOT/RFile.hxx
Outdated
| Querying this information can be done via RFile::GetKeys() or RFile::GetKeysNonRecursive. Reading an object's Key | ||
| doesn't deserialize the full object, so it's a relatively lightweight operation. | ||
| */ | ||
| struct RFileKeyInfo { |
There was a problem hiding this comment.
I'm not sure this is the best name, because the keys are the same whether it is TFile or RFile. I'd probably prefer RFile::RKeyInfo or Detail::RKeyInfo... (not entirely sure about those two either).
There was a problem hiding this comment.
I'm not sure about making it a subclass of RFile (as you say the keys are a concept of a ROOT file in general), and putting it in Detail is also a bit weird because it's not really part of the "advanced interface"...
silverweed
changed the title
silverweed
force-pushed
the
rfile_02
branch
2 times, most recently
from
7f3f1db to
d4daafe
Compare
io/io/inc/ROOT/RFile.hxx
Outdated
| std::string fName; | ||
| std::string fTitle; | ||
| std::string fClassName; | ||
| std::uint16_t fCycle = 0; |
There was a problem hiding this comment.
Here we choose to duplicate the information rather than forwarding it from the live TKey. What are the pros and cons?
There was a problem hiding this comment.
We could as well hide these and have getter methods instead, which could query the internal TKey. No reason in particular if not simplicity; for me the only important part is not to expose TKey directly, but it can make sense to use it internally to save a few copies.
I think we do want the getters in the end, so I'll change it.
EDIT: actually there is an obvious con to using the internal TKey: the RKeyInfo will have the same lifetime as the file and will become invalid when the file is closed (unless we somehow keep the TKey alive, but that I'd rather not do).
So, even though using the getters is probably better in case we want to change how it works in the future, I'd still copy the information in order to not have lifetime issues, at least for now.
io/io/src/RFile.cxx
Outdated
| if (!tfile || tfile->IsZombie()) | ||
| throw ROOT::RException(R__FAIL("failed to open file " + std::string(path) + " for reading")); | ||
|
|
||
| if (tfile->IsRaw() || !tfile->IsBinary() || tfile->IsArchive()) |
There was a problem hiding this comment.
Why is RFile sensitive to the backend (binary vs text) used by TFIle?
There was a problem hiding this comment.
Because we don't want to promise to support non-binary root files in RFile (always in case of us changing the implementation to not use TFile in the future)
silverweed
force-pushed
the
rfile_02
branch
2 times, most recently
from
f8d84c1 to
b8cb541
Compare
| - you want more robustness and better error reporting for those operations; | ||
| - you want clearer ownership semantics expressed through the type system rather than having objects "automagically" | ||
| handled for you via implicit ownership of raw pointers. | ||
| when you only need basic Put/Get operations and don't need the more advanced TFile/TDirectory functionalities. |
There was a problem hiding this comment.
Slightly off-topic, but I would prefer seeing this text in RFile.cxx or in an associated .md in the future. The reasoning is that documentation updates shouldn't recompile the world.
There was a problem hiding this comment.
The reasoning is that documentation updates shouldn't recompile the world.
In a way that makes sense, but the counter-reasoning is that a user that only has access to headers (e.g. because they installed ROOT via the system package manager) should still be able to read the documentation from them. So I'm not very convinced about it, especially considering that documentation is not expected to change frequently in the long run
There was a problem hiding this comment.
I think ROOT wasn't doing this in the past, but it's a good argument to start doing it now ...
| const auto isDir = | ||
| strcmp(key->GetClassName(), "TDirectory") == 0 || strcmp(key->GetClassName(), "TDirectoryFile") == 0; |
There was a problem hiding this comment.
Could there be TDirectory-derived types that would qualify, too, or is this forbidden by TFile?
i.e., do we also have to check if derives from TDirectory?
There was a problem hiding this comment.
I'm not entirely sure but I think there can never be directories that are not those two specific types...maybe @pcanal can answer better.
| std::vector<RKeyInfo> keys; | ||
| auto keysIter = ListKeys(); | ||
| for (const auto &key : keysIter) { | ||
| keys.emplace_back(key); |
There was a problem hiding this comment.
Consider vector.reserve() if the number of elements can be obtained.
silverweed
force-pushed
the
rfile_02
branch
2 times, most recently
from
801f6e0 to
84785ef
Compare
5 participants
Followup of #19958. Other than addressing @pcanal's comments, this PR introduces the
ListKeys()method, which iterates over the keys of some subdirectory.They do so via an iterator that returns an abridged version of TKey, currently containing only barebones information (more may be added later).
Checklist: