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

Sun Nov 22 10:12:09 PST 2020

gribozavr2 added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:525
+    return "; cannot be fixed because '" + Fixup +
+           "' is not a valid identifier";

----------------
njames93 wrote:
> gribozavr2 wrote:
> > njames93 wrote:
> > > gribozavr2 wrote:
> > > > 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"...)
> > > So just return an empty string.
> > > The user will see the normal diagnostic message but there will be no fix-its available.
> > Agreed.
> Maybe a slightly better idea is to use the same message as the empty fix up case
> `; cannot be fixed automatically`. WDYT?
> 
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91915/new/

https://reviews.llvm.org/D91915