[PATCH] D114151: [clang-format] [C++20] [Module] clang-format couldn't recognize partitions

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3226
+    // No space between module :.
+    if (Left.isOneOf(Keywords.kw_module, tok::kw_export, Keywords.kw_import) &&
+        Right.is(TT_ModulePartitionColon))
----------------
You can remove `kw_export` as it must be followed by `import` or `module`, based on how `TT_ModulePartitionColon` is set on Line 908.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3230-3231
+    // No space between import foo:bar but keep a space between import :bar;
+    if (Left.isOneOf(tok::identifier, tok::kw_public, tok::kw_private) &&
+        !Left.is(Keywords.kw_import) && Right.is(TT_ModulePartitionColon))
+      return false;
----------------
You can drop `!Left.is(Keywords.kw_import)` as `import :bar` is already covered by Line 3226. Also, I would remove `kw_public` and `kw_private` as they are not special WRT other keywords when followed by `TT_ModulePartitionColon`.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3235
+    if (Left.is(TT_ModulePartitionColon) &&
+        Right.isOneOf(tok::identifier, tok::kw_public, tok::kw_private))
+      return false;
----------------
Is `module :public` a thing in C++20? If not, I would remove `kw_public`.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3238
+    // import .....;
+    if (Left.is(Keywords.kw_import) && Right.is(tok::ellipsis))
+      return true;
----------------
You can fold this line into Line 3223 above.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1253
       }
+      if (Style.isCpp()) {
+        nextToken();
----------------
Maybe move this entire block into a function?


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1260
+          // Handle import <foo/bar.h> as we would an include statement
+          if (FormatTok && FormatTok->is(tok::less)) {
+            nextToken();
----------------
You can change this line to `else if (FormatTok->is(tok::less)) {` as `FormatTok` can't be null.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1262-1264
+            while (FormatTok) {
+              if (FormatTok->is(tok::semi))
+                break;
----------------
You can combine these three lines into one: `while (FormatTok && FormatTok->isNot(tok::semi)) {`


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1267-1268
+              // literals.
+              if (FormatTok->isNot(tok::comment) &&
+                  !FormatTok->TokenText.startswith("//"))
+                FormatTok->setType(TT_ImplicitStringLiteral);
----------------
Don't you want to mark up to the matching `>`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114151/new/

https://reviews.llvm.org/D114151