sammccall added inline comments.
================
Comment at: clang/lib/AST/Decl.cpp:4129
SourceRange TagDecl::getSourceRange() const {
- SourceLocation RBraceLoc = BraceRange.getEnd();
- SourceLocation E = RBraceLoc.isValid() ? RBraceLoc : getLocation();
+ SourceLocation E = BraceRange.getBegin().isValid() ?
+ BraceRange.getEnd() : getLocation();
----------------
hokein wrote:
> sammccall wrote:
> > I'm confused about this change.
> >
> > We were using BR.end if it was valid, now we're using it if BR.start is
> > valid. So this changes behavior in two cases:
> > - BR.start is valid, BR.end is invalid: old was {getOuterLocStart(),
> > getLocation()}. new is {getOuterLocStart(), invalid}
> > - BR.end is valid, BR.start is invalid: old was {getOuterLocStart(),
> > BraceRange.getEnd()}. new is {getOuterLocStart(), getLocation()}
> >
> > These both seem worse to me, what am I missing?
> >
> > Patch description refers to the first case, I wonder if this is just a bug
> > in clangd cancelling out a bad AST (does it just mark tokens until EOF if
> > the end location is invalid?)
> > We were using BR.end if it was valid, now we're using it if BR.start is
> > valid. So this changes behavior in two cases:
>
> > BR.start is valid, BR.end is invalid: old was {getOuterLocStart(),
> > getLocation()}. new is {getOuterLocStart(), invalid}
> > BR.end is valid, BR.start is invalid: old was {getOuterLocStart(),
> > BraceRange.getEnd()}. new is {getOuterLocStart(), getLocation()}
>
> case 1), I think this is intended, though it looks like worse at first glance
> :( Invalid source location has subtle semantic in clang -- "BR.end() is
> invalid" implies that there is no `}` in the source code.
>
> And `Missing-}` namespaceDecl in today's clang is using invalid loc (not
> the NameLoc) as the end loc:
>
> ```
> // `-NamespaceDecl 0x8d27728 </tmp/t.cpp:2:1, <invalid sloc>> col:11 abc
> namespace abc {
> // missing }
> ```
>
> case 2) is less interesting, I wonder whether we will encounter this case in
> practice, tried and failed to come up with an example to it happen. I think
> we can preserve the old behavior in this patch.
>
>
> > I wonder if this is just a bug in clangd cancelling out a bad AST (does it
> > just mark tokens until EOF if the end location is invalid?)
>
> I assume "clangd" you mean "clang"? I think clang's behavior is fine, the AST
> is recovered well, it is an issue that the TagDecl::getSourceRange doesn't
> correctly claim the range of TagDecl.
>
> e.g.
> ```
> class Test {
> int field;
> // missing }
> // `-CXXRecordDecl 0x96dd738 </tmp/t.cpp:1:1, col:7> col:7 class Test
> definition
> // |-CXXRecordDecl 0x96dd878 <col:1, col:7> col:7 implicit class Test
> // `-FieldDecl 0x96dd940 <line:2:3, col:7> col:7 field 'int'
> ```
>
> And we may not set the BraceRange.end to the EOF position -- as it changes
> the semantic, making it valid implies we have a `}` in the source code.
>
No, I meant clangd. Invalid source ranges hit codepaths in Selection.cpp that
isn't deliberately handling them, and we're getting "random" results that may
happen to do what you want in some cases, but...
e.g. mayHit returns true if the range isn't valid, which is probably good, but
the reason is bizarre: it thinks the range is from a macro expansion.
However claimRange seems to claim no tokens. So I'd guess hover, find
references etc on ABC wouldn't work after this patch, so there's regressions
there too.
Still even if it breaks clangd, the most important question here is how
*should* the source range be represented. Is a half-broken range actually
useful? Maybe it's namespacedecl that should be "fixed".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80508/new/
https://reviews.llvm.org/D80508
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
