[PATCH] D101702: [clang-format] Add more support for C# 8 nullables

[Marek Kurdej via Phabricator via cfe-commits]

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