[PATCH] D98424: [clangd] Reject renames to non-identifier characters

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:473-488
+static bool mayBeValidIdentifier(llvm::StringRef Ident) {
+  assert(llvm::json::isUTF8(Ident));
+  if (Ident.empty())
+    return false;
+  // We don't check all the rules for non-ascii characters (most are allowed).
+  bool AllowDollar = true; // lenient
+  if (llvm::isASCII(Ident.front()) &&
----------------
njames93 wrote:
> What's wrong with `isValidIdentifier` in `CharInfo.h`.
> Also isIdentifier(Body|Head) cover isASCII.
Many (most?) non-ascii characters *are* allowed.

isValidIdentifier & friends return `false` for non-ascii characters. This is OK for the fast-path of the parser, where false negatives are OK and fall back to the slow path.

We want the opposite bias: false positives are OK (allow some incorrect renames involving unicode characters) but false negatives are not (reject some valid renames)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98424