[PATCH] D112913: Misleading bidirectional detection

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Nits and a suggested approach for invalid code sequences that's probably important to handle better. Please fix the clang-tidy findings too. Otherwise, LGTM.



================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:38
+
+  /* Scan each character while maintaining a count of opened bidi context.
+   * RLO/RLE/LRO/LRE all are closed by PDF while RLI LRI and FSI are closed by
----------------
Nit: please use `//`  comments per our normal coding style.


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:49
+      ++CurPtr;
+      // line break: https://www.unicode.org/reports/tr14/tr14-32.html
+      if(C == '\n' || C == '\r' || C == '\f' || C == '\v' || C == 0x85 /*next line*/) {
----------------



================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:60-62
+    // If conversion fails, we stop the analysis because we don't know how many
+    // characters should be skipped otherwise. That's an obvious way to bypass
+    // the check.
----------------
Can we scan forward looking for the next non-continuation byte? (Skip while `c & 0b1100_0000 == 0b1000_0000`)


================
Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:74
+      Isolate = std::min(Isolate - 1, Isolate);
+    // line break: https://www.unicode.org/reports/tr14/tr14-32.html
+    else if (CodePoint == LS || CodePoint == PS)
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112913