This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Reject renames to non-identifier characters
ClosedPublic

Authored by sammccall on Mar 11 2021, 6:44 AM.

Diff Detail

Event Timeline

sammccall created this revision.Mar 11 2021, 6:44 AM
sammccall requested review of this revision.Mar 11 2021, 6:44 AM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/Rename.cpp
474–489

What's wrong with isValidIdentifier in CharInfo.h.
Also isIdentifier(Body|Head) cover isASCII.

sammccall added inline comments.Mar 11 2021, 7:24 AM
clang-tools-extra/clangd/refactor/Rename.cpp
474–489

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)

usaxena95 accepted this revision.Mar 11 2021, 9:21 AM

LGTM. Thanks.

clang-tools-extra/clangd/refactor/Rename.cpp
466

Please format the string with Reason.Details.

This revision is now accepted and ready to land.Mar 11 2021, 9:21 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 4:18 AM