This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix RenamerClangTidy checks trying to emit a fix that isnt a valid identifier
ClosedPublic

Authored by njames93 on Nov 21 2020, 9:48 AM.

Details

Diff Detail

Event Timeline

njames93 created this revision.Nov 21 2020, 9:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2020, 9:48 AM
njames93 requested review of this revision.Nov 21 2020, 9:48 AM
gribozavr2 accepted this revision.Nov 22 2020, 7:52 AM
gribozavr2 added inline comments.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
525

I don't think we need to tell that to the user. When Clang can't provide a fix, it just silently omits it. It does not help the user to know. that Clang tried, but failed.

(This message could be also read as "this code is so bad it can't be fixed"...)

This revision is now accepted and ready to land.Nov 22 2020, 7:52 AM
njames93 added inline comments.Nov 22 2020, 8:01 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
525

So just return an empty string.
The user will see the normal diagnostic message but there will be no fix-its available.

gribozavr2 added inline comments.Nov 22 2020, 8:32 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
525

Agreed.

njames93 added inline comments.Nov 22 2020, 8:50 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
525

Maybe a slightly better idea is to use the same message as the empty fix up case
; cannot be fixed automatically. WDYT?

gribozavr2 added inline comments.Nov 22 2020, 10:12 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
525

Oh I see, this checker has a number of messages like that... Well, feel free to keep it, but what seems weird to me as a user is that for an input name of "_0Bad" the checker automatically chose somehow "0_Bad" (why?), and then concluded that it is not a good choice. This looks like irrelevant information because the new name is weird. I can totally see how reminding the user that "Break"->"break" is not easily possible because the latter is a keyword.

Feel free to ignore me since this checker already produces messages like this.