Re: r287241 - Use unique_ptr for cached tokens for default arguments in C++.

[Richard Smith via cfe-commits]

On 17 November 2016 at 09:52, Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: malcolm.parsons
> Date: Thu Nov 17 11:52:58 2016
> New Revision: 287241
>
> URL: http://llvm.org/viewvc/llvm-project?rev=287241=rev
> Log:
> Use unique_ptr for cached tokens for default arguments in C++.
>
> Summary:
> 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.
>
> 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.
>
> Patch by David Tarditi!
>
> Reviewers: malcolm.parsons
>
> Subscribers: arphaman, malcolm.parsons, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D26435
>
> Modified:
> cfe/trunk/include/clang/Parse/Parser.h
> cfe/trunk/include/clang/Sema/DeclSpec.h
> cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> cfe/trunk/lib/Parse/ParseDecl.cpp
> cfe/trunk/lib/Parse/ParseDeclCXX.cpp
> cfe/trunk/lib/Sema/DeclSpec.cpp
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>
> Modified: cfe/trunk/include/clang/Parse/Parser.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
> clang/Parse/Parser.h?rev=287241=287240=287241=diff
> 
> ==
> --- cfe/trunk/include/clang/Parse/Parser.h (original)
> +++ cfe/trunk/include/clang/Parse/Parser.h Thu Nov 17 11:52:58 2016
> @@ -1017,8 +1017,8 @@ private:
>/// (C++ [class.mem]p2).
>struct LateParsedDefaultArgument {
>  explicit LateParsedDefaultArgument(Decl *P,
> -   CachedTokens *Toks = nullptr)
> -  : Param(P), Toks(Toks) { }
> +   std::unique_ptr Toks
> = nullptr)
> +  : Param(P), Toks(std::move(Toks)) { }
>
>  /// Param - The parameter declaration for this parameter.
>  Decl *Param;
> @@ -1027,7 +1027,7 @@ private:
>  /// argument expression, not including the '=' or the terminating
>  /// ')' or ','. This will be NULL for parameters that have no
>  /// default argument.
> -CachedTokens *Toks;
> +std::unique_ptr Toks;
>};
>
>/// LateParsedMethodDeclaration - A method declaration inside a class
> that
>
> Modified: cfe/trunk/include/clang/Sema/DeclSpec.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
> clang/Sema/DeclSpec.h?rev=287241=287240=287241=diff
> 
> ==
> --- cfe/trunk/include/clang/Sema/DeclSpec.h (original)
> +++ cfe/trunk/include/clang/Sema/DeclSpec.h Thu Nov 17 11:52:58 2016
> @@ -1188,14 +1188,14 @@ struct DeclaratorChunk {
>  /// declaration of a member function), it will be stored here as a
>  /// sequence of tokens to be parsed once the class definition is
>  /// complete. Non-NULL indicates that there is a default argument.
> -CachedTokens *DefaultArgTokens;
> +std::unique_ptr DefaultArgTokens;
>
>  ParamInfo() = default;
>  ParamInfo(IdentifierInfo *ident, SourceLocation iloc,
>Decl *param,
> -  CachedTokens *DefArgTokens = nullptr)
> +  std::unique_ptr DefArgTokens = nullptr)
>: Ident(ident), IdentLoc(iloc), Param(param),
> -DefaultArgTokens(DefArgTokens) {}
> +DefaultArgTokens(std::move(DefArgTokens)) {}
>};
>
>struct TypeAndRange {
> @@ -1310,10 +1310,8 @@ struct DeclaratorChunk {
>  ///
>  /// This is used in various places for error recovery.
>  void freeParams() {
> -  for (unsigned I = 0; I < NumParams; ++I) {
> -delete Params[I].DefaultArgTokens;
> -Params[I].DefaultArgTokens = nullptr;
> -  }
> +  for (unsigned I = 0; I < NumParams; ++I)
> +Params[I].DefaultArgTokens.reset();
>if (DeleteParams) {
>  delete[] Params;
>  DeleteParams = false;
>
> Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/
> ParseCXXInlineMethods.cpp?rev=287241=287240=287241=diff
> 
> ==
> --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Thu Nov 17 11:52:58 2016
> @@ -319,7 +319,8 @@ void Parser::ParseLexedMethodDeclaration
>  // Introduce the parameter into scope.
>  bool HasUnparsed = Param->hasUnparsedDefaultArg();
>  Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param);
> -if (CachedTokens *Toks = LM.DefaultArgs[I].Toks) {
> +std::unique_ptr Toks = std::move(LM.DefaultArgs[I].
> Toks);
> +if (Toks) {
>// Mark the end of the default argument so 

r287241 - Use unique_ptr for cached tokens for default arguments in C++.

[Malcolm Parsons via cfe-commits]

Author: malcolm.parsons
Date: Thu Nov 17 11:52:58 2016
New Revision: 287241

URL: http://llvm.org/viewvc/llvm-project?rev=287241=rev
Log:
Use unique_ptr for cached tokens for default arguments in C++.

Summary:
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.

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.

Patch by David Tarditi!

Reviewers: malcolm.parsons

Subscribers: arphaman, malcolm.parsons, cfe-commits

Differential Revision: https://reviews.llvm.org/D26435

Modified:
cfe/trunk/include/clang/Parse/Parser.h
cfe/trunk/include/clang/Sema/DeclSpec.h
cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
cfe/trunk/lib/Parse/ParseDecl.cpp
cfe/trunk/lib/Parse/ParseDeclCXX.cpp
cfe/trunk/lib/Sema/DeclSpec.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp

Modified: cfe/trunk/include/clang/Parse/Parser.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=287241=287240=287241=diff
==
--- cfe/trunk/include/clang/Parse/Parser.h (original)
+++ cfe/trunk/include/clang/Parse/Parser.h Thu Nov 17 11:52:58 2016
@@ -1017,8 +1017,8 @@ private:
   /// (C++ [class.mem]p2).
   struct LateParsedDefaultArgument {
 explicit LateParsedDefaultArgument(Decl *P,
-   CachedTokens *Toks = nullptr)
-  : Param(P), Toks(Toks) { }
+   std::unique_ptr Toks = 
nullptr)
+  : Param(P), Toks(std::move(Toks)) { }
 
 /// Param - The parameter declaration for this parameter.
 Decl *Param;
@@ -1027,7 +1027,7 @@ private:
 /// argument expression, not including the '=' or the terminating
 /// ')' or ','. This will be NULL for parameters that have no
 /// default argument.
-CachedTokens *Toks;
+std::unique_ptr Toks;
   };
 
   /// LateParsedMethodDeclaration - A method declaration inside a class that

Modified: cfe/trunk/include/clang/Sema/DeclSpec.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/DeclSpec.h?rev=287241=287240=287241=diff
==
--- cfe/trunk/include/clang/Sema/DeclSpec.h (original)
+++ cfe/trunk/include/clang/Sema/DeclSpec.h Thu Nov 17 11:52:58 2016
@@ -1188,14 +1188,14 @@ struct DeclaratorChunk {
 /// declaration of a member function), it will be stored here as a
 /// sequence of tokens to be parsed once the class definition is
 /// complete. Non-NULL indicates that there is a default argument.
-CachedTokens *DefaultArgTokens;
+std::unique_ptr DefaultArgTokens;
 
 ParamInfo() = default;
 ParamInfo(IdentifierInfo *ident, SourceLocation iloc,
   Decl *param,
-  CachedTokens *DefArgTokens = nullptr)
+  std::unique_ptr DefArgTokens = nullptr)
   : Ident(ident), IdentLoc(iloc), Param(param),
-DefaultArgTokens(DefArgTokens) {}
+DefaultArgTokens(std::move(DefArgTokens)) {}
   };
 
   struct TypeAndRange {
@@ -1310,10 +1310,8 @@ struct DeclaratorChunk {
 ///
 /// This is used in various places for error recovery.
 void freeParams() {
-  for (unsigned I = 0; I < NumParams; ++I) {
-delete Params[I].DefaultArgTokens;
-Params[I].DefaultArgTokens = nullptr;
-  }
+  for (unsigned I = 0; I < NumParams; ++I)
+Params[I].DefaultArgTokens.reset();
   if (DeleteParams) {
 delete[] Params;
 DeleteParams = false;

Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=287241=287240=287241=diff
==
--- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
+++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Thu Nov 17 11:52:58 2016
@@ -319,7 +319,8 @@ void Parser::ParseLexedMethodDeclaration
 // Introduce the parameter into scope.
 bool HasUnparsed = Param->hasUnparsedDefaultArg();
 Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param);
-if (CachedTokens *Toks = LM.DefaultArgs[I].Toks) {
+std::unique_ptr Toks = std::move(LM.DefaultArgs[I].Toks);
+if (Toks) {
   // Mark the end of the default argument so that we know when to stop when
   // we parse it later on.
   Token LastDefaultArgToken = Toks->back();
@@ -377,9 +378,6 @@ void Parser::ParseLexedMethodDeclaration
 
   if (Tok.is(tok::eof) && Tok.getEofData() == Param)
 ConsumeAnyToken();
-
-  delete Toks;
-  LM.DefaultArgs[I].Toks =