[PATCH] D16077: DIBuilder support DI Macro creation

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/IR/DIBuilder.cpp:94
+  for (const auto &I : AllMacrosPerParent) {
+    if (I.first == nullptr) {
+      CUNode->replaceMacros(MDTuple::get(VMContext, I.second.getArrayRef()));
----------------
if (!I.first)
Also comment what it means if it is null and why we are continuing?


================
Comment at: lib/IR/DIBuilder.cpp:101
+    TMF->getLine(), TMF->getFile(), getOrCreateMacroArray(I.second.getArrayRef()));
+    replaceTemporary(llvm::TempDIMacroNode(TMF), MF);
+  }
----------------
clang-format?


================
Comment at: lib/IR/DIBuilder.cpp:201
+  auto *M = DIMacro::get(VMContext, MacroType, LineNumber, Name, Value);
+  // Add the new DIMacro to the macro per parent map.
+  AllMacrosPerParent[Parent].insert(M);
----------------
This comment doesn't really explain anything that isn't obvious from reading the next line. Could be dropped.


================
Comment at: lib/IR/DIBuilder.cpp:210
+  auto *MF = DIMacroFile::getTemporary(VMContext, dwarf::DW_MACINFO_start_file,
+                                       LineNumber, File, Elements).release();
+  // Add the new DIFileMacro to the macro per parent map as a macro.
----------------
Why does this need to be a temporary node? Can there be cycles?


================
Comment at: lib/IR/DIBuilder.cpp:214
+  // Add the new DIFileMacro to the macro per parent map as a parent.
+  AllMacrosPerParent[MF];
+  return MF;
----------------
spelling this as AllMacrosPerParent.insert({MF, nullptr}); is perhaps more obvious?


https://reviews.llvm.org/D16077