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

[Adrian Prantl via llvm-commits]

aprantl added inline comments.

================
Comment at: lib/IR/LLVMContextImpl.h:1013
@@ +1012,3 @@
+
+  void get(unsigned ID, SmallVectorImpl<MDNode *> &Result);
+  void add(unsigned ID, MDNode &MD);
----------------
Comment mentioning what happens if the ID is invalid?

================
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);
----------------
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?

================
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:
> 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().

================
Comment at: lib/IR/LLVMContextImpl.h:1016
@@ +1015,3 @@
+  void erase(unsigned ID);
+  void getAll(SmallVectorImpl<std::pair<unsigned, MDNode *>> &Result) const;
+};
----------------
Comment explaining the sorting guarantees on the output?

================
Comment at: lib/IR/Metadata.cpp:1332-1333
@@ -1294,6 +1331,4 @@
 }
 
-void GlobalObject::setMetadata(unsigned KindID, MDNode *MD) {
-  if (MD) {
-    if (!hasMetadata())
-      setHasMetadataHashEntry(true);
+void GlobalObject::addMetadata(unsigned KindID, MDNode *MD) {
+  assert(MD);
----------------
Could/should we then change the signature to use MDNode &MD?

================
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;
----------------
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.

================
Comment at: lib/IR/Metadata.cpp:1388
@@ +1387,3 @@
+  getMetadata(KindID, MDs);
+  assert(MDs.size() <= 1);
+  if (MDs.empty())
----------------
Please add an assertion message.

================
Comment at: lib/IR/Verifier.cpp:2002
@@ -2000,1 +2001,3 @@
       case LLVMContext::MD_dbg:
+        AssertDI(++NumDebugAttachments == 1,
+                 "function must have a single !dbg attachment", &F, I.second);
----------------
Thanks! I also think I'd be more comfortable with the hoisting the increment onto its own line, given that AssertDI is a macro and all.

================
Comment at: unittests/IR/MetadataTest.cpp:2244
@@ -2243,29 +2243,3 @@
 }
 
 TEST_F(FunctionAttachmentTest, Verifier) {
----------------
Can you remind me why it is safe to drop this test without replacement?


http://reviews.llvm.org/D20414