[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

2331

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

3305

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

2331

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

2331

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

1933

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

2331

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

clang/lib/AST/Decl.cpp

3306

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

4048 ↗

(On Diff #212055)

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

3311–3314

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

3319

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

2331

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

void getNameForDiagnostic(raw_ostream &OS, const PrintingPolicy &Policy,
                             bool Qualified) const override;
 
   void setRangeEnd(SourceLocation E) { EndRangeLoc = E; }
 
   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.
   //   - it has an uninstantiated body. The body does not exist because the
   //     function is not used yet, but the declaration is considered a
   //     definition and does not allow other definition of this function.
   //   - it does not have a user specified body, but it does not allow
   //     redefinition, because it is deleted/defaulted or is defined through
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. This omits the ellipsis of a variadic function.
+  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/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 {
+  auto NP = getNumParams();
+  if (NP == 0)
+    return SourceRange();
+
+  auto Begin = ParamInfo[0]->getSourceRange().getBegin();
+  auto End = ParamInfo[NP - 1]->getSourceRange().getEnd();
+
+  return SourceRange(Begin, End);
+}
+
 SourceRange FunctionDecl::getExceptionSpecSourceRange() const {
   const TypeSourceInfo *TSI = getTypeSourceInfo();
   if (!TSI)
     return SourceRange();
   FunctionTypeLoc FTL =
     TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
   if (!FTL)
     return SourceRange();
 
   return FTL.getExceptionSpecRange();
 }
 
 /// For an inline function definition in C, or for a gnu_inline function

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, FunctionDeclSingleParameter) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 12);
+  EXPECT_TRUE(Verifier.match("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(CXXMethodDecl, CXXMethodDeclWithThrowSpecification) {
   RangeVerifier<FunctionDecl> Verifier;
   Verifier.expectRange(2, 1, 2, 16);
   EXPECT_TRUE(Verifier.match(
       "class A {\n"
       "void f() throw();\n"
       "};\n",
       functionDecl()));