MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added inline comments.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2090
continue;
+ if (Next->isOneOf(tok::star, tok::arrow))
+ continue;
----------------
sammccall wrote:
> MyDeveloperDay wrote:
> > sammccall wrote:
> > > This seems like it's supposed to be handled by the token annotator, and
> > > the same cause will result in bugs in other places - why aren't these
> > > tokens being marked as TT_OverloadedOperator?
> > >
> > > I'd guess the deeper fix is in the `kw_operator` case in consumeToken().
> > > The way it's written looks suspect, though I'm not an expert on any of
> > > this.
> > The * from *() is already labelled as a TT_PointerOrRefernce so can't also
> > be marked as an Overloaded operator so use that in the loop.
> Right, the fact that it's incorrectly labeled as a PointerOrReference is a
> bug, is it not possible to fix that instead of working around it here?
>
> In the consumeToken loop, it seems the `*` in `operator *` is labeled as
> PointerOrReference by logic that's trying to handle e.g. `StringRef::operator
> char*()`. However when the `*` is _immediately_ preceded by `operator`, it's
> always an overloaded operator name rather than a pointer, I think?
Treating `*` and `->` as an TT_OverloadedOperator is probably ok, although this
breaks unit test I highlight below, which I almost think is incorrect anyway.
I'm looking into a related issue https://bugs.llvm.org/show_bug.cgi?id=43783,
which is changing this code again, I may roll both changes into the same fix
However this is made much more complex to deal with it just by using the
`TT_OverloadedOperator` when the operator consists of 2 tokens say like `[` and
`]` unless I combine them into a single token.
```
operator void*()
operator char*()
operator []()
```
hence the need for code to handle in this loop for handling.
```
operator new[]()
operator delete[]()
```
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:955
CurrentToken->Previous->isOneOf(TT_BinaryOperator,
TT_UnaryOperator,
- tok::comma))
+ tok::comma, tok::star, tok::arrow))
CurrentToken->Previous->Type = TT_OverloadedOperator;
----------------
sammccall wrote:
> I'm confused about this: ISTM that where we were previously always treating
> star as a pointer, now we're always treating it as an operator name.
> Whereas it's sometimes an operator name (immediately after the `operator`
> keyword) and sometimes a pointer (following a type name).
>
> Maybe we should shift the OverloadedOperator labelling outside the loop
> (since AFAICT it should only apply to the first token) and then keep the loop
> to mark stars/amps elsewhere in the operator name as PointerOrReference?
Let me take another look..
================
Comment at: clang/unittests/Format/FormatTest.cpp:6989
verifyFormat("operator int();");
- verifyFormat("operator void *();");
+ verifyFormat("operator void*();");
verifyFormat("operator SomeType<int>();");
----------------
sammccall wrote:
> this looks like a regression at first glance, in LLVM style there's a space
> between type and *
yes, I was a little concerned about this change too, it's hard to know if
`operator void *` was intended as it falls off the bottom of the spacesBetween
function with a return true; but if you are concerned then perhaps its better
to change it back.
================
Comment at: clang/unittests/Format/FormatTest.cpp:6962
verifyFormat("operator int();");
verifyFormat("operator void *();");
verifyFormat("operator SomeType<int>();");
----------------
Treating * as TT_OverloadedOperator will break this test, which perhaps is
already wrong!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69573/new/
https://reviews.llvm.org/D69573
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
