<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 17 November 2016 at 09:52, Malcolm Parsons via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: malcolm.parsons<br>
Date: Thu Nov 17 11:52:58 2016<br>
New Revision: 287241<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=287241&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=287241&view=rev</a><br>
Log:<br>
Use unique_ptr for cached tokens for default arguments in C++.<br>
<br>
Summary:<br>
This changes pointers to cached tokens for default arguments in C++ from raw pointers to unique_ptrs. There was a fixme in the code where the cached tokens are created about using a smart pointer.<br>
<br>
The change is straightforward, though I did have to track down and fix a memory corruption caused by the change. memcpy was being used to copy parameter information. This duplicated the unique_ptr, which led to the cached token buffer being deleted prematurely.<br>
<br>
Patch by David Tarditi!<br>
<br>
Reviewers: malcolm.parsons<br>
<br>
Subscribers: arphaman, malcolm.parsons, cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D26435" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26435</a><br>
<br>
Modified:<br>
cfe/trunk/include/clang/Parse/<wbr>Parser.h<br>
cfe/trunk/include/clang/Sema/<wbr>DeclSpec.h<br>
cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp<br>
cfe/trunk/lib/Parse/ParseDecl.<wbr>cpp<br>
cfe/trunk/lib/Parse/<wbr>ParseDeclCXX.cpp<br>
cfe/trunk/lib/Sema/DeclSpec.<wbr>cpp<br>
cfe/trunk/lib/Sema/<wbr>SemaDeclCXX.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Parse/<wbr>Parser.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Parse/Parser.h?rev=<wbr>287241&r1=287240&r2=287241&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Parse/<wbr>Parser.h (original)<br>
+++ cfe/trunk/include/clang/Parse/<wbr>Parser.h Thu Nov 17 11:52:58 2016<br>
@@ -1017,8 +1017,8 @@ private:<br>
/// (C++ [class.mem]p2).<br>
struct LateParsedDefaultArgument {<br>
explicit LateParsedDefaultArgument(Decl *P,<br>
- CachedTokens *Toks = nullptr)<br>
- : Param(P), Toks(Toks) { }<br>
+ std::unique_ptr<CachedTokens> Toks = nullptr)<br>
+ : Param(P), Toks(std::move(Toks)) { }<br>
<br>
/// Param - The parameter declaration for this parameter.<br>
Decl *Param;<br>
@@ -1027,7 +1027,7 @@ private:<br>
/// argument expression, not including the '=' or the terminating<br>
/// ')' or ','. This will be NULL for parameters that have no<br>
/// default argument.<br>
- CachedTokens *Toks;<br>
+ std::unique_ptr<CachedTokens> Toks;<br>
};<br>
<br>
/// LateParsedMethodDeclaration - A method declaration inside a class that<br>
<br>
Modified: cfe/trunk/include/clang/Sema/<wbr>DeclSpec.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/DeclSpec.h?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Sema/DeclSpec.h?rev=<wbr>287241&r1=287240&r2=287241&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Sema/<wbr>DeclSpec.h (original)<br>
+++ cfe/trunk/include/clang/Sema/<wbr>DeclSpec.h Thu Nov 17 11:52:58 2016<br>
@@ -1188,14 +1188,14 @@ struct DeclaratorChunk {<br>
/// declaration of a member function), it will be stored here as a<br>
/// sequence of tokens to be parsed once the class definition is<br>
/// complete. Non-NULL indicates that there is a default argument.<br>
- CachedTokens *DefaultArgTokens;<br>
+ std::unique_ptr<CachedTokens> DefaultArgTokens;<br>
<br>
ParamInfo() = default;<br>
ParamInfo(IdentifierInfo *ident, SourceLocation iloc,<br>
Decl *param,<br>
- CachedTokens *DefArgTokens = nullptr)<br>
+ std::unique_ptr<CachedTokens> DefArgTokens = nullptr)<br>
: Ident(ident), IdentLoc(iloc), Param(param),<br>
- DefaultArgTokens(DefArgTokens) {}<br>
+ DefaultArgTokens(std::move(<wbr>DefArgTokens)) {}<br>
};<br>
<br>
struct TypeAndRange {<br>
@@ -1310,10 +1310,8 @@ struct DeclaratorChunk {<br>
///<br>
/// This is used in various places for error recovery.<br>
void freeParams() {<br>
- for (unsigned I = 0; I < NumParams; ++I) {<br>
- delete Params[I].DefaultArgTokens;<br>
- Params[I].DefaultArgTokens = nullptr;<br>
- }<br>
+ for (unsigned I = 0; I < NumParams; ++I)<br>
+ Params[I].DefaultArgTokens.<wbr>reset();<br>
if (DeleteParams) {<br>
delete[] Params;<br>
DeleteParams = false;<br>
<br>
Modified: cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp?rev=<wbr>287241&r1=287240&r2=287241&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp (original)<br>
+++ cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp Thu Nov 17 11:52:58 2016<br>
@@ -319,7 +319,8 @@ void Parser::<wbr>ParseLexedMethodDeclaration<br>
// Introduce the parameter into scope.<br>
bool HasUnparsed = Param->hasUnparsedDefaultArg()<wbr>;<br>
Actions.<wbr>ActOnDelayedCXXMethodParameter<wbr>(getCurScope(), Param);<br>
- if (CachedTokens *Toks = LM.DefaultArgs[I].Toks) {<br>
+ std::unique_ptr<CachedTokens> Toks = std::move(LM.DefaultArgs[I].<wbr>Toks);<br>
+ if (Toks) {<br>
// Mark the end of the default argument so that we know when to stop when<br>
// we parse it later on.<br>
Token LastDefaultArgToken = Toks->back();<br>
@@ -377,9 +378,6 @@ void Parser::<wbr>ParseLexedMethodDeclaration<br>
<br>
if (Tok.is(tok::eof) && Tok.getEofData() == Param)<br>
ConsumeAnyToken();<br>
-<br>
- delete Toks;<br>
- LM.DefaultArgs[I].Toks = nullptr;<br>
} else if (HasUnparsed) {<br>
assert(Param-><wbr>hasInheritedDefaultArg());<br>
FunctionDecl *Old = cast<FunctionDecl>(LM.Method)-<wbr>>getPreviousDecl();<br>
<br>
Modified: cfe/trunk/lib/Parse/ParseDecl.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Parse/<wbr>ParseDecl.cpp?rev=287241&r1=<wbr>287240&r2=287241&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Parse/ParseDecl.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Parse/ParseDecl.<wbr>cpp Thu Nov 17 11:52:58 2016<br>
@@ -6022,7 +6022,7 @@ void Parser::<wbr>ParseParameterDeclarationCl<br>
<br>
// DefArgToks is used when the parsing of default arguments needs<br>
// to be delayed.<br>
- CachedTokens *DefArgToks = nullptr;<br>
+ std::unique_ptr<CachedTokens> DefArgToks;<br>
<br>
// If no parameter was specified, verify that *something* was specified,<br>
// otherwise we have a missing type and identifier.<br>
@@ -6058,13 +6058,11 @@ void Parser::<wbr>ParseParameterDeclarationCl<br>
// If we're inside a class definition, cache the tokens<br>
// corresponding to the default argument. We'll actually parse<br>
// them when we see the end of the class definition.<br>
- // FIXME: Can we use a smart pointer for Toks?<br>
- DefArgToks = new CachedTokens;<br>
+ DefArgToks.reset(new CachedTokens);<br>
<br>
SourceLocation ArgStartLoc = NextToken().getLocation();<br>
if (!ConsumeAndStoreInitializer(*<wbr>DefArgToks, CIK_DefaultArgument)) {<br>
- delete DefArgToks;<br>
- DefArgToks = nullptr;<br>
+ DefArgToks.reset();<br>
Actions.<wbr>ActOnParamDefaultArgumentError<wbr>(Param, EqualLoc);<br>
} else {<br>
Actions.<wbr>ActOnParamUnparsedDefaultArgum<wbr>ent(Param, EqualLoc,<br>
@@ -6100,7 +6098,7 @@ void Parser::<wbr>ParseParameterDeclarationCl<br>
<br>
ParamInfo.push_back(<wbr>DeclaratorChunk::ParamInfo(<wbr>ParmII,<br>
ParmDeclarator.<wbr>getIdentifierLoc(),<br>
- Param, DefArgToks));<br>
+ Param, std::move(DefArgToks)));<br>
}<br>
<br>
if (TryConsumeToken(tok::<wbr>ellipsis, EllipsisLoc)) {<br>
<br>
Modified: cfe/trunk/lib/Parse/<wbr>ParseDeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDeclCXX.cpp?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Parse/<wbr>ParseDeclCXX.cpp?rev=287241&<wbr>r1=287240&r2=287241&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Parse/<wbr>ParseDeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/Parse/<wbr>ParseDeclCXX.cpp Thu Nov 17 11:52:58 2016<br>
@@ -2039,7 +2039,7 @@ void Parser::<wbr>HandleMemberFunctionDeclDel<br>
LateMethod->DefaultArgs.<wbr>reserve(FTI.NumParams);<br>
for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx)<br>
LateMethod->DefaultArgs.push_<wbr>back(<wbr>LateParsedDefaultArgument(<br>
- FTI.Params[ParamIdx].Param, FTI.Params[ParamIdx].<wbr>DefaultArgTokens));<br>
+ FTI.Params[ParamIdx].Param, std::move(FTI.Params[ParamIdx]<wbr>.DefaultArgTokens)));<br></blockquote><div><br></div><div>This line is more than 80 columns long.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/DeclSpec.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/DeclSpec.cpp?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>DeclSpec.cpp?rev=287241&r1=<wbr>287240&r2=287241&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/DeclSpec.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/DeclSpec.<wbr>cpp Thu Nov 17 11:52:58 2016<br>
@@ -223,13 +223,19 @@ DeclaratorChunk DeclaratorChunk::getFunc<br>
if (!TheDeclarator.<wbr>InlineStorageUsed &&<br>
NumParams <= llvm::array_lengthof(<wbr>TheDeclarator.InlineParams)) {<br>
I.Fun.Params = TheDeclarator.InlineParams;<br>
+ // Zero the memory block so that unique pointers are initialized<br>
+ // properly.<br>
+ memset(I.Fun.Params, 0, sizeof(Params[0]) * NumParams);<br></blockquote><div><br></div><div>This is not a reasonable way to set up the parameters. It's theoretically wrong, and practically a debug version of unique_ptr might have other members that need to be non-zero. Instead, you need to actually run the ParamInfo default constructor:</div><div><br></div><div> new (I.Fun.Params) ParamInfo[NumParams];</div><div><br></div><div>or similar.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I.Fun.DeleteParams = false;<br>
TheDeclarator.<wbr>InlineStorageUsed = true;<br>
} else {<br>
- I.Fun.Params = new DeclaratorChunk::ParamInfo[<wbr>NumParams];<br>
+ // Call the version of new that zeroes memory so that unique pointers<br>
+ // are initialized properly.<br>
+ I.Fun.Params = new DeclaratorChunk::ParamInfo[<wbr>NumParams]();<br></blockquote><div><br></div><div>You don't need the parens here. The new-expression will call the default constructor for the array element type. (There was an MSVC bug in this area but I don't *think* we support versions of MSVC with that bug any more.)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I.Fun.DeleteParams = true;<br>
}<br>
- memcpy(I.Fun.Params, Params, sizeof(Params[0]) * NumParams);<br>
+ for (unsigned i = 0; i < NumParams; i++)<br>
+ I.Fun.Params[i] = std::move(Params[i]);<br>
}<br>
<br>
// Check what exception specification information we should actually store.<br>
<br>
Modified: cfe/trunk/lib/Sema/<wbr>SemaDeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=287241&r1=287240&r2=287241&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaDeclCXX.cpp?rev=287241&r1=<wbr>287240&r2=287241&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/<wbr>SemaDeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/Sema/<wbr>SemaDeclCXX.cpp Thu Nov 17 11:52:58 2016<br>
@@ -395,7 +395,7 @@ void Sema::<wbr>CheckExtraCXXDefaultArguments<br>
++argIdx) {<br>
ParmVarDecl *Param = cast<ParmVarDecl>(chunk.Fun.<wbr>Params[argIdx].Param);<br>
if (Param->hasUnparsedDefaultArg(<wbr>)) {<br>
- CachedTokens *Toks = chunk.Fun.Params[argIdx].<wbr>DefaultArgTokens;<br>
+ std::unique_ptr<CachedTokens> Toks = std::move(chunk.Fun.Params[<wbr>argIdx].DefaultArgTokens);<br></blockquote><div><br></div><div>This line is more than 80 columns long.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
SourceRange SR;<br>
if (Toks->size() > 1)<br>
SR = SourceRange((*Toks)[1].<wbr>getLocation(),<br>
@@ -404,8 +404,6 @@ void Sema::<wbr>CheckExtraCXXDefaultArguments<br>
SR = UnparsedDefaultArgLocs[Param];<br>
Diag(Param->getLocation(), diag::err_param_default_<wbr>argument_nonfunc)<br>
<< SR;<br>
- delete Toks;<br>
- chunk.Fun.Params[argIdx].<wbr>DefaultArgTokens = nullptr;<br>
} else if (Param->getDefaultArg()) {<br>
Diag(Param->getLocation(), diag::err_param_default_<wbr>argument_nonfunc)<br>
<< Param->getDefaultArg()-><wbr>getSourceRange();<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>