Page MenuHomePhabricator

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

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



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

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.

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.


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


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.

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.


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

#define RET int
RET f(void);

Here, getReturnTypeSourceRange returns < <> <>>, where I would expect (and I could be wrong) < <> <>>

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

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.


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

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


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


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

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

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


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.