[PATCH] D15495: Macro Support in Dwarf emission

[Adrian Prantl via llvm-commits]

aprantl added a comment.

Thanks!
LGTM with the above changes addressed.


================
Comment at: include/llvm/CodeGen/DIE.h:43
@@ +42,3 @@
+  virtual unsigned emitBytes(StringRef Data) = 0;
+  // TODO: complete this interface and use it to merge EmitValue and SizeOf
+  //       methods in the DIE classes below.
----------------
Please move this comment into a doxygen comment for AbstractAsmStreamer that explains the purpose of the interface.

================
Comment at: include/llvm/CodeGen/DIE.h:48
@@ +47,3 @@
+
+/// Streams objects using an MCAsmStreamer.
+class AsmStreamEmitter : public AbstractAsmStreamer {
----------------
Please add a note that this (currently) doesn't actually report back the size.

================
Comment at: include/llvm/CodeGen/DIE.h:52
@@ +51,3 @@
+  AsmStreamEmitter(const AsmPrinter *AP);
+  virtual unsigned emitULEB128(uint64_t Value, const char *Desc = nullptr,
+                               unsigned PadTo = 0);
----------------
Probably better to use override and drop the virtual here.

================
Comment at: include/llvm/CodeGen/DIE.h:59
@@ +58,3 @@
+/// Only reports the size of the streamed objects.
+class SizeReporter : public AbstractAsmStreamer {
+public: 
----------------
Are the names

```
class AsmStreamerBase;
class EmittingAsmStreamer;
class SizeReportingAsmStreamer;
```
any better?

================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:38
@@ +37,3 @@
+
+AsmStreamEmitter::AsmStreamEmitter(const AsmPrinter *AP) :
+    AbstractAsmStreamer(AP) {
----------------
Feel free to put the one-liners into the header file, just leave the virtual destructor as an anchor.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:576
@@ -575,1 +575,3 @@
+  unsigned MacroOffset = 0;
+  std::unique_ptr<AbstractAsmStreamer> AS(new SizeReporter(Asm));
   // Handle anything that needs to be done on a per-unit basis after
----------------
Couldn't this just be a stack-allocated object?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1912
@@ +1911,3 @@
+  }
+  std::unique_ptr<AbstractAsmStreamer> AS(new AsmStreamEmitter(Asm));
+  for (const auto &P : CUMap) {
----------------
Same here. Why not just a stack-allocated object?


http://reviews.llvm.org/D15495