[PATCH] D112913: Misleading bidirectional detection

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:49
+      if (C == '\n' || C == '\r' || C == '\f' || C == '\v' ||
+          C == 0x85 /*next line*/)
+        EmbeddingOverride = Isolate = 0;
----------------
`/*next line=*/0x85` is more common


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:66
+        CodePoint == LRE)
+      EmbeddingOverride += 1;
+    else if (CodePoint == PDF)
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:68
+    else if (CodePoint == PDF)
+      EmbeddingOverride = std::min(EmbeddingOverride - 1, EmbeddingOverride);
+    else if (CodePoint == RLI || CodePoint == LRI || CodePoint == FSI)
----------------
More common style is `A = A ? A - 1 : 0;` which avoids unsigned wraparound.

That said, if the state is currently 0, should an attempt to decrease it be reported as an error as well?


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:70
+    else if (CodePoint == RLI || CodePoint == LRI || CodePoint == FSI)
+      Isolate += 1;
+    else if (CodePoint == PDI)
----------------
ditto


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:128
+
+void clang::tidy::misc::MisleadingBidirectionalCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
----------------
If you `using` MisleadingBidirectionalCheck (or `clang::tidy::misc`), then you can use

`void MisleadingBidirectionalCheck::registerMatchers`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-misleading-bidirectional.cpp:6
+  /*‮ }⁦if(admin)⁩ ⁦ begin*/
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comment contains misleading bidirectional Unicode characters [misc-misleading-bidirectional]
+  const char msg[] = "‮⁦if(admin)⁩ ⁦tes";
----------------
`[[@LINE-1]]` is a deprecated FileCheck feature.

Use `[[#@LINE-1]]`


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

https://reviews.llvm.org/D112913