jhenderson added a comment.
I'm not sure if my test comments have been looked at. Could you mark them as
Done if you think you have addressed them, please?
================
Comment at: llvm/include/llvm/ObjectYAML/DWARFYAML.h:234-237
+ Expected<uint64_t> getAbbrevTableIndexByID(uint64_t ID);
+
+private:
+ std::unordered_map<uint64_t, uint64_t> AbbrevTableID2Index;
----------------
Something to consider here: the user-visible behaviour of this class is that
this is a `const` operation, but because this method causes
`AbbrevTableID2Index` to be populated on the first call, it technically isn't.
I would say this is a reasonable use case for `mutable` - the cache (in this
case `AbbrevTableID2Index`) is marked as mutable, and then you can continue to
pass this class around as `const`.
================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:92
+ for (uint64_t I = 0; I < DI.DebugAbbrev.size(); ++I) {
+ // If the abbrev table's ID isn't specified, we use the index as its ID.
+ uint64_t ID = I;
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Maybe to avoid weird errors, it might make sense to disallow mixing the two
> > methods, i.e. if one table has an explicit ID, the others all must do too.
> > What do you think? It may not be worth it if the code is more complex, I
> > don't know.
> I think it's a little bit difficult to distinguish these 2 situations.
> Besides, sometimes we might want to add one compilation unit to a test case
> and let it reference an existing abbrev table. We don't need to mutate the
> whole test case to add IDs. What do think of it?
Leave it as is. I wasn't convinced by my own statement, so I think what you've
got is fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83116/new/
https://reviews.llvm.org/D83116
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
