[AST] Add FunctionDecl::getParametersSourceRange()

[nicolas]

Summary

This source range covers the list of parameters of a function
declaration.
This adds a SourceLocation trailing object to FunctionProtoType,
and the getEllipsisLoc and getParametersSourceRange methods to
FunctionDecl.

[nicolas]

Ping

[aaron.ballman]

clang/include/clang/AST/Decl.h

2400

Why does this omit the ellipsis? It's part of the parameter list and it seems like a strange design decision to leave that out of the source range for the parameter list.

clang/lib/AST/Decl.cpp

3360

Please don't use auto here (or below); the type is not spelled out in the initialization.

clang/unittests/AST/SourceLocationTest.cpp

676

I'd like to see tests that include an ellipsis, as well as tests that use the preprocessor in creative ways. e.g.,

#define FOO  int a, int b

void func(FOO);
void bar(float f, FOO, float g);
void baz(FOO, float f);
void quux(float f, FOO);

Also, tests for member functions (both static and instance functions) and parameters with leading attributes would be useful. e.g.,

void func([[]] int *i);

[nicolas]

• Added tests of instance and static functions
• Added tests of parameters with cv qualifiers
• Added tests of parameters with attributes
• Removed auto

[nicolas]

clang/include/clang/AST/Decl.h

2400

I haven't found a correct way to access the position of the ellipsis. There is no corresponding ParmVarDecl in ParamInfo.
Did I miss something? It doesn't seem to be easily accessible.

clang/unittests/AST/SourceLocationTest.cpp

676

Source locations with macros always looked inconsistent to me. For example:

#define RET int
RET f(void);

Here, getReturnTypeSourceRange returns <input.cc:2:1 <Spelling=input.cc:1:13>-input.cc:2:1 <Spelling=input.cc:1:13>>, where I would expect (and I could be wrong) <input.cc:2:1 <Spelling=input.cc:1:13>-input.cc:2:3 <Spelling=input.cc:1:16>>

The same thing happens with getParametersSourceRange. Should I test the current behavior?

I added the other tests you requested.

[aaron.ballman]

clang/include/clang/AST/Decl.h

2400

I don't think you've missed anything; I think we need to track/dig out that information in order for this API to be usefully correct. I don't see a lot of value in getting a source range that's sometimes wrong.

I would recommend seeing if you can go back to the function type location information as-needed to figure out where the ellipsis is and include it in the range here.

clang/unittests/AST/SourceLocationTest.cpp

676

I'd test current behavior.

[nicolas]

I added the SourceLocation of the ellipsis to FunctionProtoType in addition to the Variadic boolean.

[aaron.ballman]

clang/include/clang/AST/Decl.h

1971

Why a source range? The ellipsis is a single token, so a range should be unnecessary. I would just have this return FPT->getEllipsisLoc().

2400

Can you add "excluding the parentheses" to this as well, to make it crystal clear what is covered?

clang/lib/AST/Decl.cpp

3361

Don't use auto unless the type is explicitly spelled out in the initialization (elsewhere as well).

[nicolas]

• getEllipsisSourceRange -> getEllipsisLoc
• Updated the doc comment
• Removed auto where necessary

[riccibruno]

clang/include/clang/AST/Type.h

4056

And what if there is no ellipsis ? Shouldn't you do something like return isVariadic() ? *getTrailingObjects<SourceLocation>() : SourceLocation(); instead ?

[nicolas]

Add a default value for FunctionProtoType::getEllipsisLoc

[aaron.ballman]

clang/lib/AST/Decl.cpp

3366–3369

This isn't correct -- source locations point to the start of the token, not to the end of the token.

3374

I think this should be EllipsisLoc instead, and you can get rid of EllipsisLocEnd entirely.

[aaron.ballman]

Phab looked to be having disk issues when I added my comments, so I'm pinging this review just to be sure the latest comments make it to the mailing list.

[Mordante]

I've been looking at a way to get the ellipsis location so I'd like this patch to move forward. @nicolas are you still working on this patch?

[nicolas]

Hi @Mordante, thanks for the interest.
Here's an updated patch that addresses @aaron.ballman's comments.

[aaron.ballman]

LGTM aside from a minor commenting request.

clang/include/clang/AST/Decl.h

2400

You should make the comment clear that this will include the ellipsis (which is sort of a parameter, sort of not).

[nicolas]

Update the comment

[Mordante]

Thanks for updating the patch!

[nicolas]

Hi, if this patch still LGTY, could you commit it on my behalf?
I don't have commit access.
Thanks a lot!

[aaron.ballman]

I committed on your behalf in cc3c935da24c8ebe4fd92638574462b762d92335, thank you for the patch!

clang/include/clang/AST/Decl.h

DeclarationNameInfo getNameInfo() const {
     return DeclarationNameInfo(getDeclName(), getLocation(), DNLoc);
   }
 
   void getNameForDiagnostic(raw_ostream &OS, const PrintingPolicy &Policy,
                             bool Qualified) const override;
 
   void setRangeEnd(SourceLocation E) { EndRangeLoc = E; }
 
+  /// Returns the location of the ellipsis of a variadic function.
+  SourceLocation getEllipsisLoc() const {
+    const auto *FPT = getType()->getAs<FunctionProtoType>();
+    if (FPT && FPT->isVariadic())
+      return FPT->getEllipsisLoc();
+    return SourceLocation();
+  }
+
   SourceRange getSourceRange() const override LLVM_READONLY;
 
   // Function definitions.
   //
   // A function declaration may be:
   // - a non defining declaration,
   // - a definition. A function may be defined because:
   //   - it has a body, or will have it in the case of late parsing.
QualType getReturnType() const {
     return getType()->castAs<FunctionType>()->getReturnType();
   }
 
   /// Attempt to compute an informative source range covering the
   /// function return type. This may omit qualifiers and other information with
   /// limited representation in the AST.
   SourceRange getReturnTypeSourceRange() const;
 
+  /// Attempt to compute an informative source range covering the
+  /// function parameters, including the ellipsis of a variadic function.
+  /// The source range excludes the parentheses, and is invalid if there are
+  /// no parameters and no ellipsis.
+  SourceRange getParametersSourceRange() const;
+
   /// Get the declared return type, which may differ from the actual return
   /// type if the return type is deduced.
   QualType getDeclaredReturnType() const {
     auto *TSI = getTypeSourceInfo();
     QualType T = TSI ? TSI->getType() : getType();
     return T->castAs<FunctionType>()->getReturnType();
   }
 

clang/include/clang/AST/Type.h

 /// an exception specification, but this specification is not part of the
 /// canonical type. FunctionProtoType has several trailing objects, some of
 /// which optional. For more information about the trailing objects see
 /// the first comment inside FunctionProtoType.
 class FunctionProtoType final
     : public FunctionType,
       public llvm::FoldingSetNode,
       private llvm::TrailingObjects<
-          FunctionProtoType, QualType, FunctionType::FunctionTypeExtraBitfields,
-          FunctionType::ExceptionType, Expr *, FunctionDecl *,
-          FunctionType::ExtParameterInfo, Qualifiers> {
+          FunctionProtoType, QualType, SourceLocation,
+          FunctionType::FunctionTypeExtraBitfields, FunctionType::ExceptionType,
+          Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, Qualifiers> {
   friend class ASTContext; // ASTContext creates these.
   friend TrailingObjects;
 
   // FunctionProtoType is followed by several trailing objects, some of
   // which optional. They are in order:
   //
   // * An array of getNumParams() QualType holding the parameter types.
   //   Always present. Note that for the vast majority of FunctionProtoType,
   //   these will be the only trailing objects.
   //
+  // * Optionally if the function is variadic, the SourceLocation of the
+  //   ellipsis.
+  //
   // * Optionally if some extra data is stored in FunctionTypeExtraBitfields
   //   (see FunctionTypeExtraBitfields and FunctionTypeBitfields):
   //   a single FunctionTypeExtraBitfields. Present if and only if
   //   hasExtraBitfields() is true.
   //
   // * Optionally exactly one of:
   //   * an array of getNumExceptions() ExceptionType,
   //   * a single Expr *,
public:
   struct ExtProtoInfo {
     FunctionType::ExtInfo ExtInfo;
     bool Variadic : 1;
     bool HasTrailingReturn : 1;
     Qualifiers TypeQuals;
     RefQualifierKind RefQualifier = RQ_None;
     ExceptionSpecInfo ExceptionSpec;
     const ExtParameterInfo *ExtParameterInfos = nullptr;
+    SourceLocation EllipsisLoc;
 
     ExtProtoInfo() : Variadic(false), HasTrailingReturn(false) {}
 
     ExtProtoInfo(CallingConv CC)
         : ExtInfo(CC), Variadic(false), HasTrailingReturn(false) {}
 
     ExtProtoInfo withExceptionSpec(const ExceptionSpecInfo &ESI) {
       ExtProtoInfo Result(*this);
       Result.ExceptionSpec = ESI;
       return Result;
     }
   };
 
 private:
   unsigned numTrailingObjects(OverloadToken<QualType>) const {
     return getNumParams();
   }
 
+  unsigned numTrailingObjects(OverloadToken<SourceLocation>) const {
+    return isVariadic();
+  }
+
   unsigned numTrailingObjects(OverloadToken<FunctionTypeExtraBitfields>) const {
     return hasExtraBitfields();
   }
 
   unsigned numTrailingObjects(OverloadToken<ExceptionType>) const {
     return getExceptionSpecSize().NumExceptionType;
   }
 
public:
   ArrayRef<QualType> getParamTypes() const {
     return llvm::makeArrayRef(param_type_begin(), param_type_end());
   }
 
   ExtProtoInfo getExtProtoInfo() const {
     ExtProtoInfo EPI;
     EPI.ExtInfo = getExtInfo();
     EPI.Variadic = isVariadic();
+    EPI.EllipsisLoc = getEllipsisLoc();
     EPI.HasTrailingReturn = hasTrailingReturn();
     EPI.ExceptionSpec.Type = getExceptionSpecType();
     EPI.TypeQuals = getMethodQuals();
     EPI.RefQualifier = getRefQualifier();
     if (EPI.ExceptionSpec.Type == EST_Dynamic) {
       EPI.ExceptionSpec.Exceptions = exceptions();
     } else if (isComputedNoexcept(EPI.ExceptionSpec.Type)) {
       EPI.ExceptionSpec.NoexceptExpr = getNoexceptExpr();
public:
   /// \c ResultIfDependent.
   bool isNothrow(bool ResultIfDependent = false) const {
     return ResultIfDependent ? canThrow() != CT_Can : canThrow() == CT_Cannot;
   }
 
   /// Whether this function prototype is variadic.
   bool isVariadic() const { return FunctionTypeBits.Variadic; }
 
+  SourceLocation getEllipsisLoc() const {
+    return isVariadic() ? *getTrailingObjects<SourceLocation>()
+                        : SourceLocation();
+  }
+
   /// Determines whether this function prototype contains a
   /// parameter pack at the end.
   ///
   /// A function template whose last parameter is a parameter pack can be
   /// called with an arbitrary number of arguments, much like a variadic
   /// function.
   bool isTemplateVariadic() const;
 

clang/lib/AST/ASTContext.cpp

if (!isCanonical && Canonical.isNull()) {
     assert(!NewIP && "Shouldn't be in the map!"); (void)NewIP;
   }
 
   // Compute the needed size to hold this FunctionProtoType and the
   // various trailing objects.
   auto ESH = FunctionProtoType::getExceptionSpecSize(
       EPI.ExceptionSpec.Type, EPI.ExceptionSpec.Exceptions.size());
   size_t Size = FunctionProtoType::totalSizeToAlloc<
-      QualType, FunctionType::FunctionTypeExtraBitfields,
+      QualType, SourceLocation, FunctionType::FunctionTypeExtraBitfields,
       FunctionType::ExceptionType, Expr *, FunctionDecl *,
       FunctionProtoType::ExtParameterInfo, Qualifiers>(
-      NumArgs, FunctionProtoType::hasExtraBitfields(EPI.ExceptionSpec.Type),
+      NumArgs, EPI.Variadic,
+      FunctionProtoType::hasExtraBitfields(EPI.ExceptionSpec.Type),
       ESH.NumExceptionType, ESH.NumExprPtr, ESH.NumFunctionDeclPtr,
       EPI.ExtParameterInfos ? NumArgs : 0,
       EPI.TypeQuals.hasNonFastQualifiers() ? 1 : 0);
 
   auto *FTP = (FunctionProtoType *)Allocate(Size, TypeAlignment);
   FunctionProtoType::ExtProtoInfo newEPI = EPI;
   new (FTP) FunctionProtoType(ResultTy, ArgArray, Canonical, newEPI);
   Types.push_back(FTP);

clang/lib/AST/Decl.cpp

SourceRange FunctionDecl::getReturnTypeSourceRange() const {
   SourceLocation Boundary = getNameInfo().getBeginLoc();
   if (RTRange.isInvalid() || Boundary.isInvalid() ||
       !SM.isBeforeInTranslationUnit(RTRange.getEnd(), Boundary))
     return SourceRange();
 
   return RTRange;
 }
 
+SourceRange FunctionDecl::getParametersSourceRange() const {
+  unsigned NP = getNumParams();
+  SourceLocation EllipsisLoc = getEllipsisLoc();
+
+  if (NP == 0 && EllipsisLoc.isInvalid())
+    return SourceRange();
+
+  SourceLocation Begin =
+      NP > 0 ? ParamInfo[0]->getSourceRange().getBegin() : EllipsisLoc;
+  SourceLocation End = EllipsisLoc.isValid()
+                           ? EllipsisLoc
+                           : ParamInfo[NP - 1]->getSourceRange().getEnd();
+
+  return SourceRange(Begin, End);
+}
+
 SourceRange FunctionDecl::getExceptionSpecSourceRange() const {
   FunctionTypeLoc FTL = getFunctionTypeLoc();
   return FTL ? FTL.getExceptionSpecRange() : SourceRange();
 }
 
 /// For an inline function definition in C, or for a gnu_inline function
 /// in C++, determine whether the definition will be externally visible.
 ///

clang/lib/AST/Type.cpp

FunctionProtoType::FunctionProtoType(QualType result, ArrayRef<QualType> params,
   }
 
   if (epi.TypeQuals.hasNonFastQualifiers()) {
     FunctionTypeBits.HasExtQuals = 1;
     *getTrailingObjects<Qualifiers>() = epi.TypeQuals;
   } else {
     FunctionTypeBits.HasExtQuals = 0;
   }
+
+  // Fill in the Ellipsis location info if present.
+  if (epi.Variadic) {
+    auto &EllipsisLoc = *getTrailingObjects<SourceLocation>();
+    EllipsisLoc = epi.EllipsisLoc;
+  }
 }
 
 bool FunctionProtoType::hasDependentExceptionSpec() const {
   if (Expr *NE = getNoexceptExpr())
     return NE->isValueDependent();
   for (QualType ET : exceptions())
     // A pack expansion with a non-dependent pattern is still dependent,
     // because we don't know whether the pattern is in the exception spec

clang/lib/Sema/SemaType.cpp

case DeclaratorChunk::Function: {
           // Recover by creating a K&R-style function type.
           T = Context.getFunctionNoProtoType(T, EI);
           break;
         }
 
         FunctionProtoType::ExtProtoInfo EPI;
         EPI.ExtInfo = EI;
         EPI.Variadic = FTI.isVariadic;
+        EPI.EllipsisLoc = FTI.getEllipsisLoc();
         EPI.HasTrailingReturn = FTI.hasTrailingReturnType();
         EPI.TypeQuals.addCVRUQualifiers(
             FTI.MethodQualifiers ? FTI.MethodQualifiers->getTypeQualifiers()
                                  : 0);
         EPI.RefQualifier = !FTI.hasRefQualifier()? RQ_None
                     : FTI.RefQualifierIsLValueRef? RQ_LValue
                     : RQ_RValue;
 

clang/unittests/AST/SourceLocationTest.cpp

TEST(FunctionDecl, FunctionDeclWithNoExceptSpecification) {
   RangeVerifier<FunctionDecl> Verifier;
   Verifier.expectRange(1, 1, 1, 24);
   EXPECT_TRUE(Verifier.match(
       "void f() noexcept(false);\n",
       functionDecl(),
       Language::Lang_CXX11));
 }
 
+class FunctionDeclParametersRangeVerifier : public RangeVerifier<FunctionDecl> {
+protected:
+  SourceRange getRange(const FunctionDecl &Function) override {
+    return Function.getParametersSourceRange();
+  }
+};
+
+TEST(FunctionDeclParameters, FunctionDeclOnlyVariadic) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 8);
+  EXPECT_TRUE(Verifier.match("void f(...);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclVariadic) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 15);
+  EXPECT_TRUE(Verifier.match("void f(int a, ...);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclMacroVariadic) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 8, 1, 18);
+  EXPECT_TRUE(Verifier.match("#define VARIADIC ...\n"
+                             "void f(int a, VARIADIC);\n",
+                             functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclMacroParams) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 16, 2, 20);
+  EXPECT_TRUE(Verifier.match("#define PARAMS int a, int b\n"
+                             "void f(PARAMS, int c);",
+                             functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclSingleParameter) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 12);
+  EXPECT_TRUE(Verifier.match("void f(int a);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, MemberFunctionDecl) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 8, 2, 12);
+  EXPECT_TRUE(Verifier.match("class A{\n"
+                             "void f(int a);\n"
+                             "};",
+                             functionDecl()));
+}
+
+TEST(FunctionDeclParameters, MemberFunctionDeclVariadic) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 8, 2, 15);
+  EXPECT_TRUE(Verifier.match("class A{\n"
+                             "void f(int a, ...);\n"
+                             "};",
+                             functionDecl()));
+}
+
+TEST(FunctionDeclParameters, StaticFunctionDecl) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 15, 2, 19);
+  EXPECT_TRUE(Verifier.match("class A{\n"
+                             "static void f(int a);\n"
+                             "};",
+                             functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclMultipleParameters) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 28);
+  EXPECT_TRUE(
+      Verifier.match("void f(int a, int b, char *c);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithDefaultValue) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 16);
+  EXPECT_TRUE(Verifier.match("void f(int a = 5);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithVolatile) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 22);
+  EXPECT_TRUE(Verifier.match("void f(volatile int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithConstParam) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 19);
+  EXPECT_TRUE(Verifier.match("void f(const int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithConstVolatileParam) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 28);
+  EXPECT_TRUE(Verifier.match("void f(const volatile int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithParamAttribute) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 36);
+  EXPECT_TRUE(Verifier.match("void f(__attribute__((unused)) int a) {}",
+                             functionDecl()));
+}
+
 TEST(CXXMethodDecl, CXXMethodDeclWithThrowSpecification) {
   RangeVerifier<FunctionDecl> Verifier;
   Verifier.expectRange(2, 1, 2, 16);
   EXPECT_TRUE(Verifier.match(
       "class A {\n"
       "void f() throw();\n"
       "};\n",
       functionDecl()));