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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
