[PATCH] D132371: [DWARFLinker][NFC] Change interface of DWARFLinker to specify accel table kinds explicitly.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:33-37
 enum class DwarfLinkerAccelTableKind : uint8_t {
-  None,
   Apple,   ///< .apple_names, .apple_namespaces, .apple_types, .apple_objc.
-  Dwarf,   ///< DWARF v5 .debug_names.
-  Default, ///< Dwarf for DWARF5 or later, Apple otherwise.
-  Pub,     ///< .debug_pubnames, .debug_pubtypes
+  DWARFv4, ///< .debug_pubnames, .debug_pubtypes
+  DWARFv5  ///< .debug_names.
 };
----------------
JDevlieghere wrote:
> So now this removes "None" and "Default" and renames "Pub" to "DWARFv4"? I think this is more confusing than it was. 
> 
> If it were me I'd say:
> 
> ```
> enum class DwarfLinkerAccelTableKind : uint8_t {
>   Apple,
>   DebugNames,  // or DWARFv5 or DWARF5
>   Pub, 
> ```
> 
> On Apple platforms we want to use Apple for DWARF <=4 and debug names for >= 5.
ok.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:784
+    /// The accelerator table kinds
+    SmallVector<DwarfLinkerAccelTableKind, 8> AccelTables;
 
----------------
JDevlieghere wrote:
> I would expect a value of 1 here. 
ok.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1818-1820
+      for (const auto &Namespace : Unit.getNamespaces())
+        AppleNamespaces.addName(Namespace.Name, Namespace.Die->getOffset() +
+                                                    Unit.getStartOffset());
----------------
JDevlieghere wrote:
> Unrelated formatting changes?
It is related. The new formatting happened because nesting level was changed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132371/new/

https://reviews.llvm.org/D132371