[PATCH] D42246: [NFC] Generalize DwarfAccelTable

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 4 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp:35
+/// Return the size (in bytes) for the given Form.
+static unsigned GetSizeForForm(uint16_t Form) {
+  switch (Form) {
----------------
aprantl wrote:
> Could this be merged with DIEInteger::SizeOf() ?
I didn't know about this, sounds like a good idea. 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:163
 public:
   struct HashDataContents {
     const DIE *Die;   // Offsets
----------------
labath wrote:
> Do we need the HashDataContents struct if it is just a `DIE*` wrapper? I am not sure if there is anything that we would want to emit that cannot extracted  from a DIE pointer by a suitable DataForAtomFunction callback...
The struct will become useful when I implement the second part for dsymutil. There we don't have `DIE`s and want to track just the offsets. 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:169
 #ifndef NDEBUG
     void print(raw_ostream &OS) const {
+      OS << "  Offset: " << Die->getOffset() << "\n";
----------------
aprantl wrote:
> Is this code not needed for dwarfdump?
What do you mean exactly?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:188
+    // Third atom is a flag field that's always zero.
+    case 2:
+      return 0;
----------------
aprantl wrote:
> ``` 
>   default: return 0;
>   }
> }
> ```
> 
> not sure if actually better :-)
>From experience I know that gcc used to complain about this with `-Wall`. I kind of prefer this but I don't have a strong opinion. 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfAccelTable.h:242
 
   // Allocator for HashData and HashDataContents.
   BumpPtrAllocator Allocator;
----------------
aprantl wrote:
> Would be nice to doxygenify these comments in a separate NFC commit. (no need for a review)
Indeed. I actually have this commit ready :-) 


Repository:
  rL LLVM

https://reviews.llvm.org/D42246