This is an archive of the discontinued LLVM Phabricator instance.

Misleading identifier detection
ClosedPublic

Authored by serge-sans-paille on Nov 1 2021, 2:22 AM.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Nov 1 2021, 2:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 2:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith accepted this revision.Nov 1 2021, 3:44 PM
rsmith added a subscriber: rsmith.

Looks good as far as it goes (though I've not checked your classification functions are correct).

We should have some detection for RTL characters in string literals and comments too, at least when there is no subsequent character with strong LTR directionality. Neither " nor */ has strong directionality, so both א" and א*/ can potentially leave us in an RTL state. For example:

// This is "א" then + then "ג".
"א" + "ג"

// This is `a` then `/*א*/` then `*` then `-` then `/*ג*/` then `b`, aka "a * - b" not "a - * b".
a /*א*/ * - /*ג*/ b
clang-tools-extra/clang-tidy/misc/MisleadingIdentifier.cpp
3

Please fix

20

(throughout).

135–136

As in the other patch, we could recover here by skipping until we find a valid UTF-8 lead byte. It's not important here since invalid UTF-8 won't be accepted in an identifier anyway, but might be important if we start checking strings and comments for RTL characters too.

This revision is now accepted and ready to land.Nov 1 2021, 3:44 PM
  • fix formatting
  • added documentation
  • *not* doing the extra work wrt. unicode error recovery
This revision now requires changes to proceed.Nov 2 2021, 2:18 AM

Also update clang-tidy doc index

Update release note too.

Looks good, I guess the license issue still needs to be sorted out?

clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst
7

characters

11

example

AKAIU, the licensing issue doesn't impact that particular review, only the one on confusable identifiers.

carlosgalvezp resigned from this revision.Nov 2 2021, 5:34 AM

Ok! I don't really know what applies when you take part of a file, so I'll leave that up to people who know. I don't know how to remove the "Requested changes" from here so I'll just remove myself from reviewer.

PS: I don't know what AKAIU means, nor can I find the answer in Google :)

This revision is now accepted and ready to land.Nov 2 2021, 5:34 AM

Ok! I don't really know what applies when you take part of a file, so I'll leave that up to people who know. I don't know how to remove the "Requested changes" from here so I'll just remove myself from reviewer.

That's a valid question, but I guess that using spec content is ok... not 100% sure though.

PS: I don't know what AKAIU means, nor can I find the answer in Google :)

I meant AFAIU, I'm so skilled I can make typo in acronyms :-)

rsmith added inline comments.Nov 2 2021, 2:29 PM
clang-tools-extra/clang-tidy/misc/MisleadingIdentifier.h
3

This needs fixing too.

uabelho added a subscriber: uabelho.Nov 4 2021, 2:55 AM
rsmith accepted this revision.Nov 4 2021, 5:02 PM

I don't know how to remove the "Requested changes" from here so I'll just remove myself from reviewer.

Marking the change as accepted should overwrite the "requested changes" state.

This revision was landed with ongoing or failed builds.Nov 9 2021, 7:01 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Nov 9 2021, 8:34 AM

@serge-sans-paille Any luck with a fix for all the buildbot failures or should we revert ? https://lab.llvm.org/buildbot/#/builders/109/builds/25932

dyung added a subscriber: dyung.Nov 9 2021, 9:52 AM
aaronpuchert added inline comments.
clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst
13

Sphinx can't parse that (perhaps unsurprisingly), could you declare that as having no language?

Warning, treated as error:
/home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst:12:Could not lex literal_block as "c". Highlighting skipped.

Should be reproducible by building docs-clang-tools-html (might need -j1).

Yep, I'm currently validating locally, I somehow messed up a rebase :-/

Yep, I'm currently validating locally, I somehow messed up a rebase :-/

I'm sorry @serge-sans-paille I missed this reply before pushing the revert :(

thakis added a subscriber: thakis.Nov 9 2021, 1:34 PM

The test seems to trigger an assert: http://45.33.8.238/linux/60293/step_9.txt

Please take a look!

The test seems to trigger an assert: http://45.33.8.238/linux/60293/step_9.txt

Please take a look!

It's getting late here: Reverted, I'll have a look tomorrow.

RKSimon added inline comments.Nov 10 2021, 8:27 AM
clang-tools-extra/test/clang-tidy/checkers/misc-misleading-identifier.cpp
4

@serge-sans-paille Any chance you can remove the include and just forward declare printf()?

clang-tools-extra/test/clang-tidy/checkers/misc-misleading-identifier.cpp
4

Sure, I'll prepare a patch.

aaron.ballman added inline comments.
clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst
13

I fixed this in ac8c813b89f62efcaff4d0c92fdf1a5dd29d795d as the bot was red.