[PATCH] D68143: [clang] Make handling of unnamed template params similar to function params
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 1 03:16:10 PDT 2019
ilya-biryukov added inline comments.
================
Comment at: clang/lib/AST/DeclTemplate.cpp:513
getDefaultArgumentInfo()->getTypeLoc().getEndLoc());
- else
- return TypeDecl::getSourceRange();
+ else if(getName().empty())
+ return SourceRange(getBeginLoc());
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Could you provide more details why we need this change?
> added comments, you can also see the similar case in `DeclaratorDecl::getSourceRange()`
Could you provide an example? What the range was before and what is it now?
================
Comment at: clang/lib/Parse/ParseTemplate.cpp:633
// Grab the template parameter name (if given)
- SourceLocation NameLoc;
+ SourceLocation NameLoc = Tok.getLocation();
IdentifierInfo *ParamName = nullptr;
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > 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?
> that's same with parmvardecl case, both refer to beginning of the next token that has been lexed.
ok, LG, thanks
================
Comment at: clang/test/AST/ast-dump-record-definition-data-json.cpp:398
// CHECK-NEXT: "range": {
-// CHECK-NEXT: "begin": {
-// CHECK-NEXT: "col": 29,
-// CHECK-NEXT: "tokLen": 4
-// CHECK-NEXT: },
-// CHECK-NEXT: "end": {
-// CHECK-NEXT: "col": 29,
-// CHECK-NEXT: "tokLen": 4
-// CHECK-NEXT: }
+// CHECK-NEXT: "begin": {},
+// CHECK-NEXT: "end": {}
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Does that mean we do not have locations in some cases now?
> > Is that a regression?
> 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)
Agree, these tests do not seem to test this at all, e.g. there are other nodes which are output without any locations.
LGTM
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68143/new/
https://reviews.llvm.org/D68143
More information about the cfe-commits
mailing list