[PATCH] D20414: IR: Allow multiple global metadata attachments with the same type.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/IR/LLVMContextImpl.h:1014
@@ +1013,3 @@
+  void get(unsigned ID, SmallVectorImpl<MDNode *> &Result);
+  void add(unsigned ID, MDNode &MD);
+  void erase(unsigned ID);
----------------
aprantl wrote:
> aprantl wrote:
> > No t sure: should we define a type that inherits from std:pair<unsigned, MDNode &> and defines an operator< or would that be over-engineering?
> Usually,  we're calling this method insert().
To me it would be clearer to define the ordering just in getAll. Defining the ordering in an operator< function would imply that two attachments with the same ID are "equal", while at the moment it's clear from reading the code for getAll that that isn't the case.

================
Comment at: lib/IR/Metadata.cpp:1385
@@ -1348,3 +1384,3 @@
 
-  if (Store.empty())
-    clearMetadata();
+MDNode *GlobalObject::getMetadata(unsigned KindID) const {
+  SmallVector<MDNode *, 1> MDs;
----------------
aprantl wrote:
> The doxygen comment in the header should probably mention that this only works for singular attachments, though it's kind of obvious from the function signature.
From GlobalObject.h:
```The functions that return an MDNode require that the function have at most a
single attachment of the given kind```
:)

But I've split up the doxygen comment for getMetadata into two "blocks" so it's clearer which comments pertain to which functions.

================
Comment at: unittests/IR/MetadataTest.cpp:2244
@@ -2243,29 +2243,3 @@
 }
 
 TEST_F(FunctionAttachmentTest, Verifier) {
----------------
aprantl wrote:
> Can you remind me why it is safe to drop this test without replacement?
Its purpose is to test the GlobalObject::dropUnknownMetadata function, which has no non-test users [1], so there's no point in keeping it.
[1] http://llvm-cs.pcc.me.uk/lib/IR/Metadata.cpp/rdropUnknownMetadata


http://reviews.llvm.org/D20414