This is an archive of the discontinued LLVM Phabricator instance.

[AST] Add FunctionDecl::getParametersSourceRange()
ClosedPublic

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
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 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
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 added inline comments.Jun 27 2019, 5:02 AM
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 updated this revision to Diff 211849.Jul 25 2019, 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.Jul 26 2019, 6:50 AM
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 updated this revision to Diff 212055.Jul 27 2019, 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.Jul 27 2019, 7:53 AM
riccibruno added inline comments.Jul 27 2019, 9:37 AM
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 updated this revision to Diff 212061.Jul 27 2019, 10:08 AM

Add a default value for FunctionProtoType::getEllipsisLoc

nicolas marked an inline comment as done.Jul 27 2019, 10:08 AM
aaron.ballman added inline comments.Jul 28 2019, 8:28 AM
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.

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.

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 updated this revision to Diff 231604.Nov 30 2019, 5:24 PM

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

nicolas marked 2 inline comments as done.Nov 30 2019, 5:25 PM
nicolas marked 2 inline comments as done.Nov 30 2019, 5:30 PM
aaron.ballman accepted this revision.Dec 1 2019, 8:50 AM

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).

This revision is now accepted and ready to land.Dec 1 2019, 8:50 AM
nicolas updated this revision to Diff 231615.Dec 1 2019, 9:21 AM

Update the comment

nicolas marked an inline comment as done.Dec 1 2019, 9:21 AM

Thanks for updating the patch!

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

aaron.ballman closed this revision.Dec 3 2019, 5:23 AM

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