[PATCH] D112916: Confusable identifiers detection

MaskRay added a comment. > Subject: Confusable identifiers detection Add `[clang-tidy] ` Comment at: clang-tools-extra/clang-tidy/misc/ConfusableTable/build_confusable_table.cpp:30 + +SmallVector Values; +Line.split(Values, ';'); Place SmallVector

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille reopened this revision. serge-sans-paille added a comment. This revision is now accepted and ready to land. cc @thakis CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 ___

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille updated this revision to Diff 436399. serge-sans-paille added a comment. - quiet output of the table conversion program when everything goes well - cross-compilation support (untested) - fix identifier retrieving CHANGES SINCE LAST ACTION

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille added a comment. Thanks @thakis for the post-commit review. I'll give it another try next week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916

[PATCH] D112916: Confusable identifiers detection

thakis added a comment. Reverted in 371e6f8b7fb94c444083ba115fd8edf17d6ba05c for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916

[PATCH] D112916: Confusable identifiers detection

thakis added a comment. This also makes clang-tidy crash for fairly normal inputs, see below. Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:84 + if (const auto *ND = Result.Nodes.getNodeAs("nameddecl")) { +StringRef NDName = ND->getName(); +auto =

[PATCH] D112916: Confusable identifiers detection

thakis added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/ConfusableTable/build_confusable_table.cpp:55 + std::sort(Entries.begin(), Entries.end()); + errs() << "Parsed " << Entries.size() << " Entries\n"; + Could we not print this line? The

[PATCH] D112916: Confusable identifiers detection

thakis added a comment. I haven't checked, but this probably doesn't do the right thing in cross builds (eg building a Mac/arm binary on a Mac/Intel machine - i think the gen_confusables binary will likely end up being an arm binary then, and it won't be able to run during the build). See

[PATCH] D112916: Confusable identifiers detection

This revision was automatically updated to reflect the committed changes. Closed by commit rGb94db7ed7eaf: [clang-tidy] Confusable identifiers detection (authored by serge-sans-paille). Changed prior to commit: https://reviews.llvm.org/D112916?vs=410776=433991#toc Repository: rG LLVM Github

[PATCH] D112916: Confusable identifiers detection

Mordante added a comment. Thanks for the update! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D112916: Confusable identifiers detection

tstellar accepted this revision. tstellar added a comment. This revision is now accepted and ready to land. In D112916#3554158 , @Mordante wrote: > In D112916#3113140 , @tstellar > wrote: > >> The LLVM Board of

[PATCH] D112916: Confusable identifiers detection

Mordante added a comment. Herald added a project: All. In D112916#3113140 , @tstellar wrote: > The LLVM Board of Directors will do a legal review of this change. We will > give an update in 4-6 weeks. @tstellar, is there an update from the LLVM Board

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille updated this revision to Diff 410776. serge-sans-paille added a comment. Rebased on `main` branch and (should) fix @aaron.ballman portability issue. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 Files:

[PATCH] D112916: Confusable identifiers detection

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/ConfusableTable/build_confusable_table.cpp:22 + SmallVector Lines; + Content.split(Lines, '\n', -1, false); + We're testing this functionality in our downstream and noticed

[PATCH] D112916: Confusable identifiers detection

aaron.ballman added a comment. In D112916#3331592 , @cor3ntin wrote: > In D112916#3314184 , @cor3ntin > wrote: > >> @aaron.ballman Thanks for the ping. >> >> One one hand, I agree with you, on the other hand,

[PATCH] D112916: Confusable identifiers detection

cor3ntin added a comment. In D112916#3314184 , @cor3ntin wrote: > @aaron.ballman Thanks for the ping. > > One one hand, I agree with you, on the other hand, this tries to stick to > TR39, and I think we should stick with that. It might be worth checking

[PATCH] D112916: Confusable identifiers detection

rsmith added a comment. In D112916#3290365 , @serge-sans-paille wrote: > @rsmith : once the licensing issue has been fixed, can we merge that patch or > do you have any other thought? I have no concerns once the licensing question is resolved and the

[PATCH] D112916: Confusable identifiers detection

cor3ntin added a comment. @aaron.ballman Thanks for the ping. One one hand, I agree with you, on the other hand, this tries to stick to TR39, and I think we should stick with that. It might be worth checking with the Unicode consortium what they think of i/l as confusable. I think this is a

[PATCH] D112916: Confusable identifiers detection

aaron.ballman added reviewers: tahonermann, cor3ntin. aaron.ballman added a comment. Personally, I'm uncomfortable with this check because it's not really covering homoglyphs in general, it's covering homoglyphs outside of the usual Latin character set. For example, there's no attempt to catch

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille added a comment. @rsmith : once the licensing issue has been fixed, can we merge that patch or do you have any other thought? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 ___

[PATCH] D112916: Confusable identifiers detection

tstellar requested changes to this revision. tstellar added a comment. This revision now requires changes to proceed. The LLVM Board of Directors will do a legal review of this change. We will give an update in 4-6 weeks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille updated this revision to Diff 384854. serge-sans-paille added a comment. Portable `confusable.txt` parsing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille updated this revision to Diff 384694. serge-sans-paille added a comment. Update doc CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/ https://reviews.llvm.org/D112916 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille updated this revision to Diff 384687. serge-sans-paille added a comment. - Add documentation and changelog entry - Formatting / renaming nits - Some extra test cases - Check scope intersection in both orders CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112916/new/

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille added a comment. In D112916#3102767 , @serge-sans-paille wrote: > @tonic / @tstellar as members of the LLVM fundation board, can you tell us if > it's okay to ship the `confusables.txt` file from >

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille added subscribers: tonic, tstellar. serge-sans-paille added a comment. @tonic / @tstellar as members of the LLVM fundation board, can you tell us if it's okay to ship the `confusables.txt` file from https://www.unicode.org/Public/security/latest/confusables.txt and what are

[PATCH] D112916: Confusable identifiers detection

MaskRay added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-homoglyph.cpp:4 +int fo; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: fo is confusable with 퐟o [misc-homoglyph] +int 퐟o; `[[@LINE-1]]` is deprecated lit feature. Use

[PATCH] D112916: Confusable identifiers detection

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/CMakeLists.txt:9 +add_custom_command( +OUTPUT confusables.h +COMMAND make_confusable_table ${CMAKE_CURRENT_SOURCE_DIR}/ConfusableTable/confusables.txt ${CMAKE_CURRENT_BINARY_DIR}/confusables.h

[PATCH] D112916: Confusable identifiers detection

serge-sans-paille created this revision. Herald added subscribers: carlosgalvezp, mgrang, mgorny. serge-sans-paille requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. This patch implements detection of confusable identifiers