Page MenuHomePhabricator

[clang] Make handling of unnamed template params similar to function params
ClosedPublic

Authored by kadircet on Sep 27 2019, 8:38 AM.

Details

Summary

Clang uses the location identifier should be inserted for declarator
decls when a decl is unnamed. But for type template and template template
paramaters it uses the location of "typename/class" keyword, which makes it hard
for tooling to insert/change parameter names.

This change tries to unify these two cases by making template parameter
parsing and sourcerange operations similar to function params/declarator decls.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Sep 27 2019, 8:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 8:39 AM
ilya-biryukov added inline comments.Sep 27 2019, 11:09 AM
clang/lib/AST/DeclTemplate.cpp
513 ↗(On Diff #222183)

Could you provide more details why we need this change?

clang/lib/Parse/ParseTemplate.cpp
633 ↗(On Diff #222183)

What happens if there's a comment? E.g.

template <class /*Something*/>

where would we point to and does that align with the function parameter case?

clang/lib/Sema/SemaTemplate.cpp
1008 ↗(On Diff #222183)

NIT: maybe remove the extra variable and use ParamNameLoc everywhere?

clang/test/AST/ast-dump-record-definition-data-json.cpp
398 ↗(On Diff #222183)

Does that mean we do not have locations in some cases now?
Is that a regression?

kadircet updated this revision to Diff 222567.Oct 1 2019, 2:27 AM
kadircet marked 5 inline comments as done.
  • Address comments
clang/lib/AST/DeclTemplate.cpp
513 ↗(On Diff #222183)

added comments, you can also see the similar case in DeclaratorDecl::getSourceRange()

clang/lib/Parse/ParseTemplate.cpp
633 ↗(On Diff #222183)

that's same with parmvardecl case, both refer to beginning of the next token that has been lexed.

clang/test/AST/ast-dump-record-definition-data-json.cpp
398 ↗(On Diff #222183)

I believe not, this test is a little bit weird, if you look at the code at the beginning, it doesn't contain any "template" stuff explicitly.
So this is rather coming from implicit nodes(that doesn't have any line numbers but only column numbers)

Previously test was checking the "class" as the name of the parmdecl, hence the tokLen: 4, now it is unnamed therefore no ranges for it.
though we still have the location marker for class (col:29 tokLen: 4)

ilya-biryukov added inline comments.Oct 1 2019, 3:13 AM
clang/lib/AST/DeclTemplate.cpp
513 ↗(On Diff #222183)

Could you provide an example? What the range was before and what is it now?

clang/lib/Parse/ParseTemplate.cpp
633 ↗(On Diff #222183)

ok, LG, thanks

clang/test/AST/ast-dump-record-definition-data-json.cpp
398 ↗(On Diff #222183)

Agree, these tests do not seem to test this at all, e.g. there are other nodes which are output without any locations.
LGTM

kadircet marked an inline comment as done.Oct 1 2019, 3:30 AM
kadircet added inline comments.
clang/lib/AST/DeclTemplate.cpp
513 ↗(On Diff #222183)

TypeDecl::getSourceRange will also include the next token if template parameter is unnamed for example:

template <typename>
          ~~~~~~~~~

has the following > covered inside typerange.

kadircet marked 4 inline comments as done.Oct 1 2019, 3:30 AM
ilya-biryukov added inline comments.Oct 1 2019, 3:32 AM
clang/lib/AST/DeclTemplate.cpp
515 ↗(On Diff #222567)

getName() may fail if the name is not an identifier.
Even though this shouldn't happen for TypeDecls, could you please change to getDeclName().isEmpty()?

It's equivalent and does not have any assertions.

513 ↗(On Diff #222183)

Thanks for clearing this up. Could you mention in the comment why TypeDecl::getSourceRange is wrong in that case?

From offline discussion, I figured that it should not be pointing to the name location in the case of un-named parameters as this would lead to the closing angle bracket being included in the range for template parameter, i.e. we'll get <[[class>]] instead of <[[class]]>

kadircet marked 3 inline comments as done.Oct 1 2019, 3:40 AM
kadircet updated this revision to Diff 222581.Oct 1 2019, 3:40 AM
  • Address comments
ilya-biryukov accepted this revision.Oct 1 2019, 6:04 AM

Many thanks! LGTM

This revision is now accepted and ready to land.Oct 1 2019, 6:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 7:11 AM