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