Page MenuHomePhabricator

[AST] Add FunctionDecl::getParametersSourceRange()
Needs ReviewPublic

Authored by nicolas on Jun 13 2019, 8:19 AM.

Details

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.

Diff Detail

Event Timeline

nicolas created this revision.Jun 13 2019, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 8:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang/include/clang/AST/Decl.h
2347

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
3306

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 updated this revision to Diff 206446.Jun 25 2019, 7:51 AM
  • Added tests of instance and static functions
  • Added tests of parameters with cv qualifiers
  • Added tests of parameters with attributes
  • Removed auto
nicolas marked an inline comment as done.Jun 25 2019, 8:07 AM
nicolas added inline comments.
clang/include/clang/AST/Decl.h
2347

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 added inline comments.Jun 27 2019, 5:02 AM
clang/include/clang/AST/Decl.h
2347

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 updated this revision to Diff 211849.Thu, Jul 25, 4:21 PM
nicolas edited the summary of this revision. (Show Details)

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

aaron.ballman added inline comments.Fri, Jul 26, 6:50 AM
clang/include/clang/AST/Decl.h
1928

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

2347

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

clang/lib/AST/Decl.cpp
3307

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

nicolas updated this revision to Diff 212055.Sat, Jul 27, 7:52 AM
nicolas edited the summary of this revision. (Show Details)
  • getEllipsisSourceRange -> getEllipsisLoc
  • Updated the doc comment
  • Removed auto where necessary
nicolas marked 3 inline comments as done.Sat, Jul 27, 7:53 AM
riccibruno added inline comments.Sat, Jul 27, 9:37 AM
clang/include/clang/AST/Type.h
4033

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

nicolas updated this revision to Diff 212061.Sat, Jul 27, 10:08 AM

Add a default value for FunctionProtoType::getEllipsisLoc

nicolas marked an inline comment as done.Sat, Jul 27, 10:08 AM
aaron.ballman added inline comments.Sun, Jul 28, 8:28 AM
clang/lib/AST/Decl.cpp
3312–3315

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

3320

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

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.