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

[Adrian Prantl via llvm-commits]

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

One last thing: If there isn't one already, there should be a unit test for the uniquing.
LGTM with these points addressed. Thanks!


================
Comment at: lib/IR/DIBuilder.cpp:131
@@ -126,1 +130,3 @@
+  if (!ImportedModules.empty())
+    CUNode->replaceImportedEntities(MDTuple::get(VMContext, ImportedModules));
 
----------------
aaboud wrote:
> 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?
I think the answer is yes. DIBuilder::finalize() makes this assumption here and will happily replace the contents of all of these arrays, so we should take the opportunity to guard against misuse of the interface by asserting that they are empty before replacing them.

================
Comment at: lib/IR/DIBuilder.cpp:178
@@ -177,3 +181,1 @@
-    // Add it to the Imported Modules list.
-    AllImportedModules.emplace_back(M);
   return M;
----------------
aaboud wrote:
> 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.
That makes sense. I didn't look at the type AllImportedModules.


http://reviews.llvm.org/D19468