[PATCH] D132224: [Assignment Tracking][5/*] Add core infrastructure for instruction reference

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfo.h:163
+/// Assignment Tracking (at).
+namespace at {
+//
----------------
I'm bikeshedding but i think the namespace name is too terse. Also possibly not nice to read  because it's a word and an acronym


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:803
+    if (isa<DIAssignID>(Init)) {
+      for (auto *Inst : TempDIAssignIDAttachments[ToReplace]) {
+        assert(!Inst->getMetadata(LLVMContext::MD_DIAssignID) &&
----------------
Possibly need to assert on ToReplace?


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:805
+        assert(!Inst->getMetadata(LLVMContext::MD_DIAssignID) &&
+               "Inst already has DIAssignID?");
+        Inst->setMetadata(LLVMContext::MD_DIAssignID, Init);
----------------
nit, Is assert text really a question? Can you just state instead?


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

https://reviews.llvm.org/D132224