rymiel added inline comments.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:3592-3601
case tok::kw_bool:
// bool is only allowed if it is directly followed by a paren for a cast:
// concept C = bool(...);
// and bool is the only type, all other types as cast must be inside a
// cast to bool an thus are handled by the other cases.
if (Tokens->peekNextToken()->isNot(tok::l_paren))
return;
----------------
owenpan wrote:
> owenpan wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > rymiel wrote:
> > > > > owenpan wrote:
> > > > > > rymiel wrote:
> > > > > > > rymiel wrote:
> > > > > > > > owenpan wrote:
> > > > > > > > > We should pass `true` to `peekNextToken()` on Line 3597 here,
> > > > > > > > > but removing this entire case would have no effect on the
> > > > > > > > > existing unit test (Line 23693 in FormatTest.cpp).
> > > > > > > > > @HazardyKnusperkeks any idea?
> > > > > > > > The peeking is there because of
> > > > > > > > https://reviews.llvm.org/D134325, it has no format tests, only
> > > > > > > > an annotator test
> > > > > > > Also, I don't think that function is involved in concept
> > > > > > > declarations since https://reviews.llvm.org/D140339
> > > > > > So which tests were meant to check the `kw_bool` case here?
> > > > > Well, originally the one you mentioned on line 23693, but since the
> > > > > code paths have been changed, I think it's really only the one added
> > > > > in https://reviews.llvm.org/D134325
> > > > I love the new annotator tests, but we should probably always try to
> > > > add a verifyFormat test as well even if its trivial in the future, just
> > > > so we have some context as to what type of code warranted the change.
> > > As far as I can see it does not only can be removed, it should be removed.
> > >
> > > `bool` can not appear on the top level of a requires clause. As @rymiel
> > > said it is there for the concepts, which are now handled differently.
> > @rymiel @HazardyKnusperkeks thanks for the explanations. Can we remove the
> > `kw_bool` case then?
> > As far as I can see it does not only can be removed, it should be removed.
>
> Okay, I will leave it to you or @rymiel then. :)
> As far as I can see it does not only can be removed, it should be removed.
You are right! I forgot the fact that this only ever applied for concepts, and
the bug fix only ever worked around that. I had it backwards in my head because
the bug affected requires clauses too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142412/new/
https://reviews.llvm.org/D142412
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
