curdeius requested changes to this revision. curdeius added a comment. This revision now requires changes to proceed.
Looks very good already. Just some questions and test requests. Requesting changes so it shows up nicely in the revision queue. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:1593 + tok::kw_namespace, tok::r_paren, tok::r_square, tok::r_brace, + tok::kw_false, tok::kw_true, Keywords.kw_type, Keywords.kw_get, + Keywords.kw_set) || ---------------- Does adding false and true here may negatively affect JS in any way? (Just thinking out loud) ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:3195 + // No space before null forgiving '!'. + if (Right.is(TT_JsNonNullAssertion)) + return false; ---------------- Should it be renamed to include C#? ================ Comment at: clang/unittests/Format/FormatTestCSharp.cpp:365 + + verifyFormat("test \?\?= ABC;", Style); + verifyFormat("test \?\?= true;", Style); ---------------- exv wrote: > HazardyKnusperkeks wrote: > > Why do you escape `?`? I've never seen that. > Apparently, ??= is a trigraph for "#" in C, so it must be escaped: > > https://en.wikipedia.org/wiki/Digraphs_and_trigraphs#C > > To be honest, I'd never heard of this bizarre language "feature" until I > wrote this patch. Not only in C but in C++ too. Fortunately removed in C++17. Nobody misses trigraphs... ================ Comment at: clang/unittests/Format/FormatTestCSharp.cpp:380 + verifyFormat("test = !bar! \?\? !foo!;"); + verifyFormat("bool test = !(!true || !null! || !false && !false! && !bar()! " + "+ (!foo()))!"); ---------------- Please test also `true!` or `!true!`. ================ Comment at: clang/unittests/Format/FormatTestCSharp.cpp:381 + verifyFormat("bool test = !(!true || !null! || !false && !false! && !bar()! " + "+ (!foo()))!"); +} ---------------- Could you test that `!` is correctly kept together with the corresponding identifier around the line breaks, please? You might adjust ColumnLimit for that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101702/new/ https://reviews.llvm.org/D101702 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits