[PATCH] D68143: [clang] Make handling of unnamed template params similar to function params

[Ilya Biryukov via Phabricator via cfe-commits]

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