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