[PATCH] D74158: make the AsmPrinterHandler array public

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

Looks good, thanks! Please implement the suggestions at your discretion before landing.



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinterHandler.h:41
 
+  virtual void beginModule(Module *M){};
+
----------------
Please remove the extra semicolon and reformat to add the space before the brace.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1642
   }
-  Handlers.clear();
+  Handlers.erase(Handlers.begin() + NumUserHandlers, Handlers.end());
   DD = nullptr;
----------------
Please add a comment here elaborating a bit on this. This is deleting all the handlers that AsmPrinter added, while keeping all the user-added handlers alive until the AsmPrinter is destroyed.


================
Comment at: llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp:404
+    PM.add(new MachineModuleInfoWrapperPass(LLVMTM));
+    PM.add(AP); // Takes ownership of AP
+    LLVMContext Context;
----------------
This is test code, but to have one less owning raw pointer, I suggest passing `TestPrinter->releaseAP()` here directly, and initialize the above AP variable with `getAP()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74158