<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 28, 2014 at 9:31 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ctopper<br>
Date: Fri Nov 28 23:31:10 2014<br>
New Revision: 222947<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=222947&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=222947&view=rev</a><br>
Log:<br>
Use unique_ptr to remove some explicit deletes on some error case returns. At least one spot of weird ownership passing that needs some future cleanup.<br>
<br>
Modified:<br>
llvm/trunk/lib/TableGen/TGParser.cpp<br>
<br>
Modified: llvm/trunk/lib/TableGen/TGParser.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?rev=222947&r1=222946&r2=222947&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?rev=222947&r1=222946&r2=222947&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/TableGen/TGParser.cpp (original)<br>
+++ llvm/trunk/lib/TableGen/TGParser.cpp Fri Nov 28 23:31:10 2014<br>
@@ -340,26 +340,20 @@ bool TGParser::ProcessForeachDefs(Record<br>
// This is the bottom of the recursion. We have all of the iterator values<br>
// for this point in the iteration space. Instantiate a new record to<br>
// reflect this combination of values.<br>
- Record *IterRec = new Record(*CurRec);<br>
+ auto IterRec = make_unique<Record>(*CurRec);<br>
<br>
// Set the iterator values now.<br>
for (unsigned i = 0, e = IterVals.size(); i != e; ++i) {<br>
VarInit *IterVar = IterVals[i].IterVar;<br>
TypedInit *IVal = dyn_cast<TypedInit>(IterVals[i].IterValue);<br>
- if (!IVal) {<br>
- Error(Loc, "foreach iterator value is untyped");<br>
- delete IterRec;<br>
- return true;<br>
- }<br>
+ if (!IVal)<br>
+ return Error(Loc, "foreach iterator value is untyped");<br>
<br>
IterRec->addValue(RecordVal(IterVar->getName(), IVal->getType(), false));<br>
<br>
- if (SetValue(IterRec, Loc, IterVar->getName(),<br>
- std::vector<unsigned>(), IVal)) {<br>
- Error(Loc, "when instantiating this def");<br>
- delete IterRec;<br>
- return true;<br>
- }<br>
+ if (SetValue(IterRec.get(), Loc, IterVar->getName(),<br>
+ std::vector<unsigned>(), IVal))<br>
+ return Error(Loc, "when instantiating this def");<br>
<br>
// Resolve it next.<br>
IterRec->resolveReferencesTo(IterRec->getValue(IterVar->getName()));<br>
@@ -370,17 +364,15 @@ bool TGParser::ProcessForeachDefs(Record<br>
<br>
if (Records.getDef(IterRec->getNameInitAsString())) {<br>
// If this record is anonymous, it's no problem, just generate a new name<br>
- if (IterRec->isAnonymous())<br>
- IterRec->setName(GetNewAnonymousName());<br>
- else {<br>
- Error(Loc, "def already exists: " + IterRec->getNameInitAsString());<br>
- delete IterRec;<br>
- return true;<br>
- }<br>
+ if (!IterRec->isAnonymous())<br>
+ return Error(Loc, "def already exists: " +IterRec->getNameInitAsString());<br>
+<br>
+ IterRec->setName(GetNewAnonymousName());<br>
}<br>
<br>
- Records.addDef(IterRec);<br>
- IterRec->resolveReferences();<br>
+ Record *IterRecSave = IterRec.get(); // Keep a copy before release.<br>
+ Records.addDef(IterRec.release());<br></blockquote><div><br>Could we simplify this further by having the map in Records have a Record value rather than a unique_ptr<Record> or Record* value? I guess we'd have to move the addDef up to the top, maybe roll the name/anonymous name collision handling into that function(?) and have it return a raw pointer or reference to the element (maybe return a null pointer for the "def already exists" error case).<br><br>Possibly similarly with the MultiClass stuff below, but it's a bit trickier, as you mentioned.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ IterRecSave->resolveReferences();<br>
return false;<br>
}<br>
<br>
@@ -1247,26 +1239,26 @@ Init *TGParser::ParseSimpleValue(Record<br>
SMLoc EndLoc = Lex.getLoc();<br>
<br>
// Create the new record, set it as CurRec temporarily.<br>
- Record *NewRec = new Record(GetNewAnonymousName(), NameLoc, Records,<br>
- /*IsAnonymous=*/true);<br>
+ auto NewRecOwner = make_unique<Record>(GetNewAnonymousName(), NameLoc,<br>
+ Records, /*IsAnonymous=*/true);<br>
+ Record *NewRec = NewRecOwner.get(); // Keep a copy since we may release.<br>
SubClassReference SCRef;<br>
SCRef.RefRange = SMRange(NameLoc, EndLoc);<br>
SCRef.Rec = Class;<br>
SCRef.TemplateArgs = ValueList;<br>
// Add info about the subclass to NewRec.<br>
- if (AddSubClass(NewRec, SCRef)) {<br>
- delete NewRec;<br>
+ if (AddSubClass(NewRec, SCRef))<br>
return nullptr;<br>
- }<br>
+<br>
if (!CurMultiClass) {<br>
NewRec->resolveReferences();<br>
- Records.addDef(NewRec);<br>
+ Records.addDef(NewRecOwner.release());<br>
} else {<br>
// This needs to get resolved once the multiclass template arguments are<br>
// known before any use.<br>
NewRec->setResolveFirst(true);<br>
// Otherwise, we're inside a multiclass, add it to the multiclass.<br>
- CurMultiClass->DefPrototypes.push_back(NewRec);<br>
+ CurMultiClass->DefPrototypes.push_back(NewRecOwner.release());<br>
<br>
// Copy the template arguments for the multiclass into the def.<br>
const std::vector<Init *> &TArgs =<br>
@@ -2036,27 +2028,23 @@ bool TGParser::ParseDef(MultiClass *CurM<br>
Lex.Lex(); // Eat the 'def' token.<br>
<br>
// Parse ObjectName and make a record for it.<br>
- Record *CurRec;<br>
- bool CurRecOwnershipTransferred = false;<br>
+ std::unique_ptr<Record> CurRecOwner;<br>
Init *Name = ParseObjectName(CurMultiClass);<br>
if (Name)<br>
- CurRec = new Record(Name, DefLoc, Records);<br>
+ CurRecOwner = make_unique<Record>(Name, DefLoc, Records);<br>
else<br>
- CurRec = new Record(GetNewAnonymousName(), DefLoc, Records,<br>
- /*IsAnonymous=*/true);<br>
+ CurRecOwner = make_unique<Record>(GetNewAnonymousName(), DefLoc, Records,<br>
+ /*IsAnonymous=*/true);<br>
+ Record *CurRec = CurRecOwner.get(); // Keep a copy since we may release.<br>
<br>
if (!CurMultiClass && Loops.empty()) {<br>
// Top-level def definition.<br>
<br>
// Ensure redefinition doesn't happen.<br>
- if (Records.getDef(CurRec->getNameInitAsString())) {<br>
- Error(DefLoc, "def '" + CurRec->getNameInitAsString()<br>
- + "' already defined");<br>
- delete CurRec;<br>
- return true;<br>
- }<br>
- Records.addDef(CurRec);<br>
- CurRecOwnershipTransferred = true;<br>
+ if (Records.getDef(CurRec->getNameInitAsString()))<br>
+ return Error(DefLoc, "def '" + CurRec->getNameInitAsString()+<br>
+ "' already defined");<br>
+ Records.addDef(CurRecOwner.release());<br>
<br>
if (ParseObjectBody(CurRec))<br>
return true;<br>
@@ -2066,24 +2054,17 @@ bool TGParser::ParseDef(MultiClass *CurM<br>
// before this object, instantiated prior to defs derived from this object,<br>
// and this available for indirect name resolution when defs derived from<br>
// this object are instantiated.<br>
- if (ParseObjectBody(CurRec)) {<br>
- delete CurRec;<br>
+ if (ParseObjectBody(CurRec))<br>
return true;<br>
- }<br>
<br>
// Otherwise, a def inside a multiclass, add it to the multiclass.<br>
for (unsigned i = 0, e = CurMultiClass->DefPrototypes.size(); i != e; ++i)<br>
if (CurMultiClass->DefPrototypes[i]->getNameInit()<br>
- == CurRec->getNameInit()) {<br>
- Error(DefLoc, "def '" + CurRec->getNameInitAsString() +<br>
- "' already defined in this multiclass!");<br>
- delete CurRec;<br>
- return true;<br>
- }<br>
- CurMultiClass->DefPrototypes.push_back(CurRec);<br>
- CurRecOwnershipTransferred = true;<br>
+ == CurRec->getNameInit())<br>
+ return Error(DefLoc, "def '" + CurRec->getNameInitAsString() +<br>
+ "' already defined in this multiclass!");<br>
+ CurMultiClass->DefPrototypes.push_back(CurRecOwner.release());<br>
} else if (ParseObjectBody(CurRec)) {<br>
- delete CurRec;<br>
return true;<br>
}<br>
<br>
@@ -2109,15 +2090,10 @@ bool TGParser::ParseDef(MultiClass *CurM<br>
}<br>
<br>
if (ProcessForeachDefs(CurRec, DefLoc)) {<br>
- Error(DefLoc,<br>
- "Could not process loops for def" + CurRec->getNameInitAsString());<br>
- if (!CurRecOwnershipTransferred)<br>
- delete CurRec;<br>
- return true;<br>
+ return Error(DefLoc, "Could not process loops for def" +<br>
+ CurRec->getNameInitAsString());<br>
}<br>
<br>
- if (!CurRecOwnershipTransferred)<br>
- delete CurRec;<br>
return false;<br>
}<br>
<br>
@@ -2416,22 +2392,21 @@ InstantiateMulticlassDef(MultiClass &MC,<br>
// Make a trail of SMLocs from the multiclass instantiations.<br>
SmallVector<SMLoc, 4> Locs(1, DefmPrefixRange.Start);<br>
Locs.append(DefProto->getLoc().begin(), DefProto->getLoc().end());<br>
- Record *CurRec = new Record(DefName, Locs, Records, IsAnonymous);<br>
+ auto CurRec = make_unique<Record>(DefName, Locs, Records, IsAnonymous);<br>
<br>
SubClassReference Ref;<br>
Ref.RefRange = DefmPrefixRange;<br>
Ref.Rec = DefProto;<br>
- AddSubClass(CurRec, Ref);<br>
+ AddSubClass(CurRec.get(), Ref);<br>
<br>
// Set the value for NAME. We don't resolve references to it 'til later,<br>
// though, so that uses in nested multiclass names don't get<br>
// confused.<br>
- if (SetValue(CurRec, Ref.RefRange.Start, "NAME", std::vector<unsigned>(),<br>
- DefmPrefix)) {<br>
+ if (SetValue(CurRec.get(), Ref.RefRange.Start, "NAME",<br>
+ std::vector<unsigned>(), DefmPrefix)) {<br>
Error(DefmPrefixRange.Start, "Could not resolve "<br>
+ CurRec->getNameInitAsString() + ":NAME to '"<br>
+ DefmPrefix->getAsUnquotedString() + "'");<br>
- delete CurRec;<br>
return nullptr;<br>
}<br>
<br>
@@ -2463,14 +2438,17 @@ InstantiateMulticlassDef(MultiClass &MC,<br>
Error(DefmPrefixRange.Start, "def '" + CurRec->getNameInitAsString() +<br>
"' already defined, instantiating defm with subdef '" +<br>
DefProto->getNameInitAsString() + "'");<br>
- delete CurRec;<br>
return nullptr;<br>
}<br>
<br>
- Records.addDef(CurRec);<br>
+ Record *CurRecSave = CurRec.get(); // Keep a copy before we release.<br>
+ Records.addDef(CurRec.release());<br>
+ return CurRecSave;<br>
}<br>
<br>
- return CurRec;<br>
+ // FIXME This is bad but the ownership transfer to caller is pretty messy.<br>
+ // The unique_ptr in this function at least protects the exits above.<br>
+ return CurRec.release();<br>
}<br>
<br>
bool TGParser::ResolveMulticlassDefArgs(MultiClass &MC,<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>