hokein marked an inline comment as done.
hokein 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();
----------------
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.
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
