[PATCH] D16248: [Clang-tidy] rename misc-inefficient-algorithm to performance-inefficient-algorithm

[Aaron Ballman via cfe-commits]

On Tue, Jan 19, 2016 at 10:20 AM, Alexander Kornienko <alexfh at google.com> wrote:
> alexfh added inline comments.
>
> ================
> Comment at: clang-tidy/misc/MiscTidyModule.cpp:58
> @@ -57,3 +56,1 @@
> -    CheckFactories.registerCheck<InefficientAlgorithmCheck>(
> -        "misc-inefficient-algorithm");
>      CheckFactories.registerCheck<MacroParenthesesCheck>(
> ----------------
> aaron.ballman wrote:
>> alexfh wrote:
>> > alexfh wrote:
>> > > aaron.ballman wrote:
>> > > > This will break projects that enable the misc-inefficient-algorithm check (which clang 3.7 exposed). Is there a reason to not keep this check registered under this name?
>> > > >
>> > > > (Perhaps a follow-up patch to allow deprecation of check names so that users are given guidance would make sense.)
>> > > I don't feel strongly, but I'm somewhat reluctant to keep old check names. With every new clang-tidy version someone starts using on a project, they need to carefully look at the list of checks and select relevant ones anyway. I think, adding facilities for deprecating checks and keeping old names is not going to help much, but will certainly add support burden for us.
>> > But we certainly need to mention the rename in the release notes for 3.8.
>> > I don't feel strongly, but I'm somewhat reluctant to keep old check names. With every new clang-tidy version someone starts using on a project, they need to carefully look at the list of checks and select relevant ones anyway. I think, adding facilities for deprecating checks and keeping old names is not going to help much, but will certainly add support burden for us.
>>
>> I'm more worried about upgrading existing uses than initiating new uses on a project. If my build system enabled this check for my project, then upgrading clang-tidy will cause that build to break because of an unknown check name, won't it? I think it's reasonable to do that if there's compelling reason (e.g., we remove a check entirely because it's no longer useful for some reason), but I'd like to avoid gratuitously breaking changes because it adds a barrier to people's upgrade paths.
>>
>> Oye. I just tested this out and the results were...surprisingly unhelpful.
>> ```
>> e:\llvm\2015>clang-tidy -checks=misc-hahahaha-nope E:\Desktop\test.cpp --
>> e:\llvm\2015>
>> ```
>> So it seems we don't currently diagnose providing unknown check names at all, which would make this a silently breaking change (existing uses will no longer trigger the check *and* they won't trigger any diagnostic mentioning that the check isn't known). :-(
>> If my build system enabled this check for my project, then upgrading clang-tidy will cause that build to break because of an unknown check name, won't it?
>
> Only in one case: when you have just one check enabled. Clang-tidy's -checks= option is a **filter**, not a **list**, so it can't detect a presence of invalid check names there. We could add this detection, probably (e.g. if removal of a glob from the list doesn't change anything), and issue a warning, but there is no reason to fail hard, when the check filter contains invalid entries, IMO.

The user wrote something and likely assumed it had an effect when it
doesn't have one -- that doesn't seem like intuitive (or particularly
useful) behavior as far as the user is concerned. Typos are easy
mistakes to make ("is inefficient spelled 'inefficient' or
'inefficeint', that whole i before e thing is so tricky"), and the
problem here is that it can be hard for a user to detect when they've
messed up the filter because it's impossible to tell the difference
between "check never ran" and "my code is perfect."

~Aaron