[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

Thu Aug 20 03:02:43 PDT 2020

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