[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348145: [AST][Sema] Remove CallExpr::setNumArgs (authored by 
brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54902?vs=175434&id=176394#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54902/new/

https://reviews.llvm.org/D54902

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp

Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -2416,11 +2416,12 @@
   // These versions of the constructor are for derived classes.
   CallExpr(const ASTContext &C, StmtClass SC, Expr *fn,
ArrayRef preargs, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 0);
   CallExpr(const ASTContext &C, StmtClass SC, Expr *fn, ArrayRef args,
-   QualType t, ExprValueKind VK, SourceLocation rparenloc);
+   QualType t, ExprValueKind VK, SourceLocation rparenloc,
+   unsigned MinNumArgs = 0);
   CallExpr(const ASTContext &C, StmtClass SC, unsigned NumPreArgs,
-   EmptyShell Empty);
+   unsigned NumArgs, EmptyShell Empty);
 
   Stmt *getPreArg(unsigned i) {
 assert(i < getNumPreArgs() && "Prearg access out of range!");
@@ -2438,11 +2439,14 @@
   unsigned getNumPreArgs() const { return CallExprBits.NumPreArgs; }
 
 public:
-  CallExpr(const ASTContext& C, Expr *fn, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+  /// Build a call expression. MinNumArgs specifies the minimum number of
+  /// arguments. The actual number of arguments will be the greater of
+  /// args.size() and MinNumArgs.
+  CallExpr(const ASTContext &C, Expr *fn, ArrayRef args, QualType t,
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 0);
 
   /// Build an empty call expression.
-  CallExpr(const ASTContext &C, StmtClass SC, EmptyShell Empty);
+  CallExpr(const ASTContext &C, unsigned NumArgs, EmptyShell Empty);
 
   const Expr *getCallee() const { return cast(SubExprs[FN]); }
   Expr *getCallee() { return cast(SubExprs[FN]); }
@@ -2488,10 +2492,18 @@
 SubExprs[Arg+getNumPreArgs()+PREARGS_START] = ArgExpr;
   }
 
-  /// setNumArgs - This changes the number of arguments present in this call.
-  /// Any orphaned expressions are deleted by this, and any new operands are set
-  /// to null.
-  void setNumArgs(const ASTContext& C, unsigned NumArgs);
+  /// Reduce the number of arguments in this call expression. This is used for
+  /// example during error recovery to drop extra arguments. There is no way
+  /// to perform the opposite because: 1.) We don't track how much storage
+  /// we have for the argument array 2.) This would potentially require growing
+  /// the argument array, something we cannot support since the arguments will
+  /// be stored in a trailing array in the future.
+  /// (TODO: update this comment when this is done).
+  void shrinkNumArgs(unsigned NewNumArgs) {
+assert((NewNumArgs <= NumArgs) &&
+   "shrinkNumArgs cannot increase the number of arguments!");
+NumArgs = NewNumArgs;
+  }
 
   typedef ExprIterator arg_iterator;
   typedef ConstExprIterator const_arg_iterator;
Index: cfe/trunk/include/clang/AST/ExprCXX.h
===
--- cfe/trunk/include/clang/AST/ExprCXX.h
+++ cfe/trunk/include/clang/AST/ExprCXX.h
@@ -98,8 +98,10 @@
 Range = getSourceRangeImpl();
   }
 
-  explicit CXXOperatorCallExpr(ASTContext& C, EmptyShell Empty)
-  : CallExpr(C, CXXOperatorCallExprClass, Empty) {}
+  explicit CXXOperatorCallExpr(ASTContext &C, unsigned NumArgs,
+   EmptyShell Empty)
+  : CallExpr(C, CXXOperatorCallExprClass, /*NumPreArgs=*/0, NumArgs,
+ Empty) {}
 
   /// Returns the kind of overloaded operator that this
   /// expression refers to.
@@ -163,12 +165,13 @@
 /// the object argument).
 class CXXMemberCallExpr : public CallExpr {
 public:
-  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args,
-QualType t, ExprValueKind VK, SourceLocation RP)
-  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP) {}
+  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, QualType t,
+ExprValueKind VK, SourceLocation RP,
+unsigned MinNumArgs = 0)
+  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP, MinNumArgs) {}
 
-  CXXMemberCallExpr(ASTContext &C, EmptyShell Empty)
-  : CallExpr(C, CXXMemberCallExprCl

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from two small NFC nits.




Comment at: include/clang/AST/ExprCXX.h:168
 public:
-  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args,
-QualType t, ExprValueKind VK, SourceLocation RP)
-  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP) {}
+  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, QualType t,
+ExprValueKind VK, SourceLocation RP,

riccibruno wrote:
> aaron.ballman wrote:
> > Since you're already touching the line, can you correct the names `fn`, 
> > `args`, and `t` to match our naming conventions?
> I was going to submit an NFC cleaning up all of `CallExpr` +
> the classes deriving from it in one go (capitalization style,
> `clang-format`, and so on).
> 
> I would prefer to avoid mixing style fixes in existing code
> and functional changes.
That's fine by me, thanks!



Comment at: lib/Sema/SemaExpr.cpp:5607
-  Fn = rewrite.get();
-  TheCall->setCallee(Fn);
-  goto retry;

riccibruno wrote:
> aaron.ballman wrote:
> > Why did this go away?
> Because it is now set by the constructor of `CallExpr` or
> `CUDAKernelCallExpr`. The call to `setCallee` was needed
> before because the call expression was constructed before
> this piece of code, but now we can just pass `Fn` to the
> constructor.
Thank you for the explanation, that makes sense to me.



Comment at: lib/Sema/SemaExpr.cpp:5625
 
   // Bail out early if calling a builtin with custom typechecking.
   if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID))

typechecking -> type checking



Comment at: lib/Serialization/ASTReaderStmt.cpp:2499
+  S = new (Context) CallExpr(
+  Context, /* NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);

I would drop the `+ 0` from all of these; they look a bit like a typo from a 
casual reading of the code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54902/new/

https://reviews.llvm.org/D54902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

added inline comments




Comment at: include/clang/AST/ExprCXX.h:168
 public:
-  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args,
-QualType t, ExprValueKind VK, SourceLocation RP)
-  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP) {}
+  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, QualType t,
+ExprValueKind VK, SourceLocation RP,

aaron.ballman wrote:
> Since you're already touching the line, can you correct the names `fn`, 
> `args`, and `t` to match our naming conventions?
I was going to submit an NFC cleaning up all of `CallExpr` +
the classes deriving from it in one go (capitalization style,
`clang-format`, and so on).

I would prefer to avoid mixing style fixes in existing code
and functional changes.



Comment at: lib/Sema/SemaExpr.cpp:5607
-  Fn = rewrite.get();
-  TheCall->setCallee(Fn);
-  goto retry;

aaron.ballman wrote:
> Why did this go away?
Because it is now set by the constructor of `CallExpr` or
`CUDAKernelCallExpr`. The call to `setCallee` was needed
before because the call expression was constructed before
this piece of code, but now we can just pass `Fn` to the
constructor.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54902/new/

https://reviews.llvm.org/D54902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 175434.
riccibruno marked 9 inline comments as done.
riccibruno added a comment.

style fixes


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54902/new/

https://reviews.llvm.org/D54902

Files:
  include/clang/AST/Expr.h
  include/clang/AST/ExprCXX.h
  lib/AST/Expr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderStmt.cpp

Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -731,10 +731,11 @@
 
 void ASTStmtReader::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
-  E->setNumArgs(Record.getContext(), Record.readInt());
+  unsigned NumArgs = Record.readInt();
+  assert((NumArgs == E->getNumArgs()) && "Wrong NumArgs!");
   E->setRParenLoc(ReadSourceLocation());
   E->setCallee(Record.readSubExpr());
-  for (unsigned I = 0, N = E->getNumArgs(); I != N; ++I)
+  for (unsigned I = 0; I != NumArgs; ++I)
 E->setArg(I, Record.readSubExpr());
 }
 
@@ -2494,7 +2495,9 @@
   break;
 
 case EXPR_CALL:
-  S = new (Context) CallExpr(Context, Stmt::CallExprClass, Empty);
+  S = new (Context) CallExpr(
+  Context, /* NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_MEMBER: {
@@ -3084,11 +3087,15 @@
 }
 
 case EXPR_CXX_OPERATOR_CALL:
-  S = new (Context) CXXOperatorCallExpr(Context, Empty);
+  S = new (Context) CXXOperatorCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_CXX_MEMBER_CALL:
-  S = new (Context) CXXMemberCallExpr(Context, Empty);
+  S = new (Context) CXXMemberCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_CXX_CONSTRUCT:
@@ -3128,7 +3135,8 @@
   break;
 
 case EXPR_USER_DEFINED_LITERAL:
-  S = new (Context) UserDefinedLiteral(Context, Empty);
+  S = new (Context) UserDefinedLiteral(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], Empty);
   break;
 
 case EXPR_CXX_STD_INITIALIZER_LIST:
@@ -3303,7 +3311,8 @@
   break;
 
 case EXPR_CUDA_KERNEL_CALL:
-  S = new (Context) CUDAKernelCallExpr(Context, Empty);
+  S = new (Context) CUDAKernelCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], Empty);
   break;
 
 case EXPR_ASTYPE:
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -12836,7 +12836,8 @@
 
 CXXMemberCallExpr *call
   = new (Context) CXXMemberCallExpr(Context, MemExprE, Args,
-resultType, valueKind, RParenLoc);
+resultType, valueKind, RParenLoc,
+proto->getNumParams());
 
 if (CheckCallReturnType(proto->getReturnType(), op->getRHS()->getBeginLoc(),
 call, nullptr))
@@ -12986,9 +12987,11 @@
   ResultType = ResultType.getNonLValueExprType(Context);
 
   assert(Method && "Member call to something that isn't a method?");
+  const auto *Proto = Method->getType()->getAs();
   CXXMemberCallExpr *TheCall =
 new (Context) CXXMemberCallExpr(Context, MemExprE, Args,
-ResultType, VK, RParenLoc);
+ResultType, VK, RParenLoc,
+Proto->getNumParams());
 
   // Check for a valid return type.
   if (CheckCallReturnType(Method->getReturnType(), MemExpr->getMemberLoc(),
@@ -13008,8 +13011,6 @@
   }
 
   // Convert the rest of the arguments
-  const FunctionProtoType *Proto =
-Method->getType()->getAs();
   if (ConvertArgumentsForCall(TheCall, MemExpr, Method, Proto, Args,
   RParenLoc))
 return ExprError();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -4857,7 +4857,10 @@
 
   return true;
 }
-Call->setNumArgs(Context, NumParams);
+// We reserve space for the default arguments when we create
+// the call expression, before calling ConvertArgumentsForCall.
+assert((Call->getNumArgs() == NumParams) &&
+   "We should have reserved space for the default arguments before!");
   }
 
   // If too many are passed and not variadic, error on the extras and drop
@@ -4898,7 +4901,7 @@
 Diag(FDecl->getBeginLoc(), diag::note_callee_decl) << FDecl;
 
   // This deletes the extra arguments.
-  Call->setNumArgs(Context, NumParams);
+  Call->shrinkNumArgs(NumParams);
   return true;
 }
   }
@@ -5558,17 +5561,51 @@
 return ExprError();
   Fn = Result

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ExprCXX.h:168
 public:
-  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args,
-QualType t, ExprValueKind VK, SourceLocation RP)
-  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP) {}
+  CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, QualType t,
+ExprValueKind VK, SourceLocation RP,

Since you're already touching the line, can you correct the names `fn`, `args`, 
and `t` to match our naming conventions?



Comment at: include/clang/AST/ExprCXX.h:212
   CUDAKernelCallExpr(ASTContext &C, Expr *fn, CallExpr *Config,
- ArrayRef args, QualType t, ExprValueKind VK,
- SourceLocation RP)
-  : CallExpr(C, CUDAKernelCallExprClass, fn, Config, args, t, VK, RP) {}
+ ArrayRef args, QualType t, ExprValueKind VK,
+ SourceLocation RP, unsigned MinNumArgs = 0)

Same here.



Comment at: lib/AST/Expr.cpp:1272
ArrayRef preargs, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc)
+   ExprValueKind VK, SourceLocation rparenloc,
+   unsigned MinNumArgs)

If you want to fix these up as well, feel free.



Comment at: lib/Sema/SemaExpr.cpp:5565
+  // Check for a valid function type, but only if it is not a builtin which
+  // requires custom typechecking. These will be handled by
+  // CheckBuiltinFunctionCall below just after creation of the call expression.

typechecking -> type checking



Comment at: lib/Sema/SemaExpr.cpp:5594
+
+  // Get the number of parameter in the function prototype, if any.
+  // We will allocate space for max(Args.size(), NumParams) arguments

parameter -> parameters



Comment at: lib/Sema/SemaExpr.cpp:5607
-  Fn = rewrite.get();
-  TheCall->setCallee(Fn);
-  goto retry;

Why did this go away?



Comment at: lib/Sema/SemaOverload.cpp:12990
   assert(Method && "Member call to something that isn't a method?");
+  const FunctionProtoType *Proto =
+Method->getType()->getAs();

You can use `const auto *` here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54902/new/

https://reviews.llvm.org/D54902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: rsmith, aaron.ballman.
riccibruno added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith, mehdi_amini.

`CallExpr::setNumArgs` is the only thing that prevents storing the arguments
in a trailing array. There is only 3 places in `Sema` where `setNumArgs` is 
called.
D54900  dealt with one of them.

This patch remove the other two calls to `setNumArgs` in 
`ConvertArgumentsForCall`.
To do this we do the following changes:

1.) Replace the first call to `setNumArgs` by an assertion since we are moving 
the responsability
to allocate enough space for the arguments from `Sema::ConvertArgumentsForCall`
to its callers. (which are `Sema::BuildCallToMemberFunction`, and 
`Sema::BuildResolvedCallExpr`)

2.) Add a new member function `CallExpr::shrinkNumArgs`, which can only be used
to drop arguments and then replace the second call to `setNumArgs` by 
`shrinkNumArgs`.

3.) Add a new defaulted parameter `MinNumArgs` to `CallExpr` and its derived 
classes,
which specifies a minimum number of argument slots to allocate. The actual 
number of
arguments slots allocated will be `max(number of args, MinNumArgs)`, with the 
extra
args nulled. Note that after the creation of the call expression all of the 
arguments will
be non-null. It is just during the creation of the call expression that some of 
the
last arguments can be temporarily null, until filled by default arguments.

4.) Update `Sema::BuildCallToMemberFunction` by passing the number of parameters
in the function prototype to the constructor of `CXXMemberCallExpr`.
Here the change is pretty straightforward.

5.) Update `Sema::BuildResolvedCallExpr`. Here the change is more complicated 
since
the type-checking for the function type was done after the creation of the call 
expression.
We need to move this before the creation of the call expression, and then pass 
the number
of parameters in the function prototype (if any) to the constructor of the call 
expression.

6.) Update the deserialization of `CallExpr` and its derived classes.


Repository:
  rC Clang

https://reviews.llvm.org/D54902

Files:
  include/clang/AST/Expr.h
  include/clang/AST/ExprCXX.h
  lib/AST/Expr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderStmt.cpp

Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -731,10 +731,11 @@
 
 void ASTStmtReader::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
-  E->setNumArgs(Record.getContext(), Record.readInt());
+  unsigned NumArgs = Record.readInt();
+  assert((NumArgs == E->getNumArgs()) && "Wrong NumArgs!");
   E->setRParenLoc(ReadSourceLocation());
   E->setCallee(Record.readSubExpr());
-  for (unsigned I = 0, N = E->getNumArgs(); I != N; ++I)
+  for (unsigned I = 0; I != NumArgs; ++I)
 E->setArg(I, Record.readSubExpr());
 }
 
@@ -2494,7 +2495,9 @@
   break;
 
 case EXPR_CALL:
-  S = new (Context) CallExpr(Context, Stmt::CallExprClass, Empty);
+  S = new (Context) CallExpr(
+  Context, /* NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_MEMBER: {
@@ -3084,11 +3087,15 @@
 }
 
 case EXPR_CXX_OPERATOR_CALL:
-  S = new (Context) CXXOperatorCallExpr(Context, Empty);
+  S = new (Context) CXXOperatorCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_CXX_MEMBER_CALL:
-  S = new (Context) CXXMemberCallExpr(Context, Empty);
+  S = new (Context) CXXMemberCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Empty);
   break;
 
 case EXPR_CXX_CONSTRUCT:
@@ -3128,7 +3135,8 @@
   break;
 
 case EXPR_USER_DEFINED_LITERAL:
-  S = new (Context) UserDefinedLiteral(Context, Empty);
+  S = new (Context) UserDefinedLiteral(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], Empty);
   break;
 
 case EXPR_CXX_STD_INITIALIZER_LIST:
@@ -3303,7 +3311,8 @@
   break;
 
 case EXPR_CUDA_KERNEL_CALL:
-  S = new (Context) CUDAKernelCallExpr(Context, Empty);
+  S = new (Context) CUDAKernelCallExpr(
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], Empty);
   break;
 
 case EXPR_ASTYPE:
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -12836,7 +12836,8 @@
 
 CXXMemberCallExpr *call
   = new (Context) CXXMemberCallExpr(Context, MemExprE, Args,
-resultType, valueKind, RParenLoc);
+resultType, valueKind, RParenLoc,
+proto->getNumParams())