[PATCH] D116793: [AST] Add more source information for DecltypeTypeLoc.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D116793#3227175 <https://reviews.llvm.org/D116793#3227175>, @hokein wrote:

>> (BTW the parsing part of this is probably adjacent to a fix for https://github.com/clangd/clangd/issues/121, AutoType also doesn't have enough locations for decltype(auto). But definitely not something to touch in this patch)
>
> Yeah, that is my next step, this is an extending change of `AutoTypeLoc`.

Yay :-)



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1012
+    // token doesn't have it.
+    DS.setTypeofParensRange(SourceRange(SourceLocation(), EndLoc));
     ConsumeAnnotationToken();
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > This is unfortunate for a common case `decltype(expression) a;`, the `ParseDecltypeSpecifier` is called twice to parse the decltype specifier: 
> > > 
> > > - first time, we parse the raw decltype token, and we annotate that (the `DS` was created locally, and then was thrown away)
> > > - second time, we parsed the annotated decltype, and set another `DS`, at this point, we lost the LParen information :(
> > > 
> > > One way to fix that is to add a new field `SourceLocation` in `Token` class (there is 4-bytes padding size left, so it won't increase its size), but I'm a little worried, it seems like an overkill to just fix this issue. 
> > > 
> > > 
> > What do you think about only storing the kw loc + endloc then?
> > This is enough to compute the correct sourcerange, which was the original bug we hit.
> > 
> > Our options are:
> >  - do the work to always compute it: nice to have but it does seem hard/intrusive
> >  - don't store/expose it
> >  - expose it but have it be unreliable
> > 
> > I don't know if 3 is better than 2 (without a use case it's hard to judge), maybe it just encourages writing buggy code. And if nothing else, 2 is smaller :-)
> Yeah, having the decltype loc + rparen loc should be enough to fix our bug, and it might be a better idea than having a partial working `getLParenLoc`.
> 
> > I don't know if 3 is better than 2 (without a use case it's hard to judge), maybe it just encourages writing buggy code. And if nothing else, 2 is smaller :-)
> 
> We don't have a concrete use case of the paren range of `decltype`, so 2 is probably the best (though having a getRParenLoc API without a paired getLParenLoc feels incomplete).
Agree. Would it make sense to keep the internal names concrete but expose publicly only as begin/endLoc?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116793/new/

https://reviews.llvm.org/D116793