[PATCH] D80508: [AST] Fix the source range for TagDecl if there is no matched } brace.

[Haojian Wu via Phabricator via cfe-commits]

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