HazardyKnusperkeks requested changes to this revision. HazardyKnusperkeks added a comment. This revision now requires changes to proceed.
In D130299#3672486 <https://reviews.llvm.org/D130299#3672486>, @denis-fatkulin wrote: >> Could you please add full git context? > > I updated the patch with properly git context. Thanks! > >> Was the problem due to misannotation of auto? If so, could you add an >> annotator test? > > I'm not sure about the questions. I will try to explain my patch purpose. > Actually there was two problems: > > 1. `auto` wasn't detected as properly type keyword in lambda's trailing > return type. So, formatting was completely wrong for this case (fixed in > `clang/lib/Format/UnwrappedLineParser.cpp`) > 2. The keyword `auto` and left brace `{` was interpreted as declaration > `auto{}`. So, formatting delete a space symbol between them. (fixed in > `clang/lib/Format/TokenAnnotator.cpp`) > > Both cases are checked in changes at `clang/unittests/Format/FormatTest.cpp` > and I think the unit test is sufficient. The question is, wether `auto` did get the type `kw_auto` before your change or not. If not a test for the token annotator would be in place. See e.g. D129946 <https://reviews.llvm.org/D129946>. And I actually think you should amend the test case from there, because as far as I can see if there was an `auto` we would stop parsing the lambda and not assign the `TT_LambdaLBrace`, etc. In fact I can't see the anything related to braced lists in your fixes. The error was primarily an annotation error, with the adding space as a formation error. The fucked up braced list is just a follow up error. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:3314 + // Lambda with trailing return type 'auto': []() -> auto { return T; } + if (Left.is(tok::kw_auto) && Right.getType() == TT_LambdaLBrace) + return true; ---------------- ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:3314 + // Lambda with trailing return type 'auto': []() -> auto { return T; } + if (Left.is(tok::kw_auto) && Right.getType() == TT_LambdaLBrace) + return true; ---------------- HazardyKnusperkeks wrote: > Maybe split it up in two changes, and change it to this, because I think we would have the same problem. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130299/new/ https://reviews.llvm.org/D130299 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
