<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jan 7, 2010, at 11:39 AM, Devang Patel wrote:</div><blockquote type="cite"><div>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=92931&view=rev">http://llvm.org/viewvc/llvm-project?rev=92931&view=rev</a><br>Log:<br>Use separate namespace for named metadata.<br></div></blockquote><div><br></div>Great!</div><div><br><blockquote type="cite"><div>+++ llvm/trunk/include/llvm/Metadata.h Thu Jan 7 13:39:36 2010<br></div></blockquote><br><blockquote type="cite"><div>+ /// setName - Set the name of this named metadata.<br>+ void setName(StringRef Name);<br></div></blockquote><div><br></div>If setName is part of the public API for this class then it should ensure that the module symtab is kept up to date. Right now, setName will change the NamedMDNode but not the symtab afaict.</div><div><br></div><div><blockquote type="cite"><div>+++ llvm/trunk/include/llvm/Module.h Thu Jan 7 13:39:36 2010<br>@@ -322,6 +324,10 @@<br> /// NamedMDNode with the specified name is not found.<br> NamedMDNode *getOrInsertNamedMetadata(StringRef Name);<br><br>+ /// addMDNodeName - Insert an entry in the NamedMDNode symbol table mapping<br>+ /// Name to NMD. <br>+ void addMDNodeName(StringRef Name, NamedMDNode *NMD);<br></div></blockquote><div><br></div><div>This should not be public API.</div><br><blockquote type="cite"><div>+++ llvm/trunk/include/llvm/ValueSymbolTable.h Thu Jan 7 13:39:36 2010<br>@@ -129,6 +129,84 @@<br> /// @}<br> };<br><br>+/// This class provides a symbol table of name/NamedMDNode pairs. It is <br>+/// essentially a StringMap wrapper.<br>+<br>+class MDSymbolTable {<br></div></blockquote><blockquote type="cite"><div>+/// @name Types<br>+/// @{<br>+public:<br>+ /// @brief A mapping of names to metadata<br>+ typedef StringMap<NamedMDNode*> MDMap;<br></div></blockquote><div><br></div>This typedef should not be public. Please also disable copy ctor and operator=.</div><div><br><blockquote type="cite"><div>+public:<br>+ /// insert - The method inserts a new entry into the stringmap.<br>+ void insert(StringRef Name, NamedMDNode *Node) {<br>+ (void) mmap.GetOrCreateValue(Name, Node);<br>+ }<br>+ <br>+ /// This method removes a NamedMDNode from the symbol table. <br>+ void remove(StringRef Name) { mmap.erase(Name); }<br></div></blockquote><div><br></div><div>Should these be private? Random classes should interact with the symbol table through Module and the NamedMDNode constructor.</div><div><br></div><blockquote type="cite"><div>+++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp Thu Jan 7 13:39:36 2010<br>@@ -528,10 +528,9 @@<br> }<br><br> // Write name.<br>- std::string Str = NMD->getNameStr();<br>- const char *StrBegin = Str.c_str();<br>- for (unsigned i = 0, e = Str.length(); i != e; ++i)<br>- Record.push_back(StrBegin[i]);<br>+ StringRef Str = NMD->getName();<br>+ for (unsigned i = 0, e = Str.size(); i != e; ++i)<br>+ Record.push_back(Str[i]);<br> Stream.EmitRecord(bitc::METADATA_NAME, Record, 0/*TODO*/);<br> Record.clear();<br></div></blockquote><div><br></div><div>This is a fine change. However, coming back to the bitcode encoding of NamedMDNodes, why is this split into two records? It seems that <span class="Apple-style-span" style="font-family: Menlo; font-size: 11px; ">METADATA_NAME / METADATA_NAMED_NODE could be one record.</span></div><div><font class="Apple-style-span" face="Menlo" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font></div><div><font class="Apple-style-span" face="Menlo" size="3"><span class="Apple-style-span" style="font-size: 11px;"><span class="Apple-style-span" style="font-family: Helvetica; font-size: medium; ">The first element of the record would be the # MDNodes, then that # of mdnode id's would follow, then a length of the name, then the characters of the name.</span></span></font></div><div><br></div><blockquote type="cite"><div>+++ llvm/trunk/lib/VMCore/Metadata.cpp Thu Jan 7 13:39:36 2010<br>@@ -214,20 +214,21 @@<br> return *(SmallVector<WeakVH, 4>*)Operands;<br> }<br><br>-NamedMDNode::NamedMDNode(LLVMContext &C, const Twine &N,<br>+NamedMDNode::NamedMDNode(LLVMContext &C, StringRef N,<br></div></blockquote><div><br></div>Shouldn't this take a Twine, not a StringRef for consistency with everything else?</div><div><br><blockquote type="cite"><div> MDNode *const *MDs, <br> unsigned NumMDs, Module *ParentModule)<br> : MetadataBase(Type::getMetadataTy(C), Value::NamedMDNodeVal), Parent(0) {<br> setName(N);<br>- <br> Operands = new SmallVector<WeakVH, 4>();<br><br> SmallVector<WeakVH, 4> &Node = getNMDOps(Operands);<br> for (unsigned i = 0; i != NumMDs; ++i)<br> Node.push_back(WeakVH(MDs[i]));<br><br>- if (ParentModule)<br>+ if (ParentModule) {<br> ParentModule->getNamedMDList().push_back(this);<br>+ ParentModule->addMDNodeName(N, this);<br></div></blockquote><div><br></div><div>If you set up the ilist_traits right, you don't need to do this. The traits should autoinsert the NamedMDNode into the symbol table when it is added to a module, and autoremove it from the symtab when it is removed from the module.</div><br><blockquote type="cite"><div>@@ -265,6 +266,7 @@<br> /// eraseFromParent - Drop all references and remove the node from parent<br> /// module.<br> void NamedMDNode::eraseFromParent() {<br>+ getParent()->getMDSymbolTable().remove(getName());<br> getParent()->getNamedMDList().erase(this);<br></div></blockquote><div><br></div>Likewise.</div><div><br><blockquote type="cite"><div> }<br><br>@@ -273,6 +275,16 @@<br> getNMDOps(Operands).clear();<br> }<br><br>+/// setName - Set the name of this named metadata.<br>+void NamedMDNode::setName(StringRef N) {<br>+ if (!N.empty())<br>+ Name = N.str();<br></div></blockquote><div><br></div><div>Should assert that N != empty. All NamedMDNodes must have non-empty names.</div><br><blockquote type="cite"><div>+++ llvm/trunk/lib/VMCore/Module.cpp Thu Jan 7 13:39:36 2010<br>@@ -59,6 +59,7 @@<br> : Context(C), ModuleID(MID), DataLayout("") {<br> ValSymTab = new ValueSymbolTable();<br> TypeSymTab = new TypeSymbolTable();<br>+ NamedMDSymTab = new MDSymbolTable();<br></div></blockquote><div><br></div>You added a new, but not a delete to the destructor. Please don't leak this.</div><div><br></div>-Chris</body></html>