Page MenuHomePhabricator

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

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

Details

Diff Detail

Event Timeline

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

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.Sun, Nov 22, 7:52 AM
njames93 added inline comments.Sun, Nov 22, 8:01 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
526

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.Sun, Nov 22, 8:32 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
526

Agreed.

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

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.Sun, Nov 22, 10:12 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
526

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.