curdeius requested changes to this revision. curdeius added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/lib/Format/UnwrappedLineParser.cpp:613 + // settings. This allows the format to back up one level in those cases. + if (!AddLevel && NamespaceBlock && + Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) ---------------- HazardyKnusperkeks wrote: > Maybe give `!AddLevel && Style.BreakBeforeBraces == > FormatStyle::BS_Whitesmiths` a name? You check it three times. Agree on this. ================ Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2159 + + // If we're in whitesmiths mode, indent the brace. + if (!AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) ---------------- Nit: please write consistently Whitesmiths with a capital W. ================ Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2163 + + parseBlock(/*MustBeDeclaration=*/true, AddLevel, true, true); // Munch the semicolon after a namespace. This is more common than one would ---------------- 3*true? Please add comments a minima. ================ Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2259 + // If in Whitesmiths mode, the line with the while() needs to be indented + // to the same level as the block + if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) ---------------- Nit: full stop at the end of the phrase. ================ Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2967 + // needs to be indented. + bool ClosesBlock = + Line->MatchingOpeningBlockLineIndex != UnwrappedLine::kInvalidIndex && ---------------- Please move below to the place of use. Also, just naming it `ClosesBlock` is a bit misleading as it is only for Whitesmiths style. ================ Comment at: clang/unittests/Format/FormatTest.cpp:13688 - WhitesmithsBraceStyle.IndentCaseBlocks = true; + WhitesmithsBraceStyle.IndentCaseLabels = true; verifyFormat("void switchTest1(int a)\n" ---------------- Hmm, you don't test the same thing anymore... ================ Comment at: clang/unittests/Format/FormatTest.cpp:13696 " }\n" - " break;\n" + " break;\n" " }\n" ---------------- MyDeveloperDay wrote: > whilst I don't really like changing the indentation I don't think there was > anything that says it was correct before. That is really strange that `case` is indented differently than `break`. Is it intended? Is the style really like this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94500/new/ https://reviews.llvm.org/D94500 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits