[PATCH] D19468: Disallow duplication of imported entities (improved implementation)

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Thanks Adrian for the comments, please see answers below.


================
Comment at: lib/IR/DIBuilder.cpp:131
@@ -126,1 +130,3 @@
+  if (!ImportedModules.empty())
+    CUNode->replaceImportedEntities(MDTuple::get(VMContext, ImportedModules));
 
----------------
aprantl wrote:
> Maybe assert that CUNode->getImportedEntities() is empty?
I do not mind adding assertion, but how this is different than all the other replace function, e.g. the one at line 120.
Should we add assertion for all that the list was empty before replacing it with the actual values?

================
Comment at: lib/IR/DIBuilder.cpp:178
@@ -177,3 +181,1 @@
-    // Add it to the Imported Modules list.
-    AllImportedModules.emplace_back(M);
   return M;
----------------
aprantl wrote:
> This is more a general question: Is there a difference between emplace_back for a *pointer* element and a regular push_back?
push_back assumes you are adding element of type T, where T in our case is "TrackingMDNodeRef".
emplace_back will take the element(s) as arguments that can be passed to the T constructor during allocation.

I guess we could call the push_back like this:
AllImportedModules.emplace_back(TrackingMDNodeRef(M));

At least this is my understanding.
Also notice that I am just reverting this part from commit 263379.


http://reviews.llvm.org/D19468