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

[Alexander Kornienko via cfe-commits]

On Tue, Jan 19, 2016 at 4:28 PM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> 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."
>

That's why I say clang-tidy could issue a warning, if a glob list fed to
-checks= has entries that have no effect. The only question is who is
bothered enough by this and has time to implement this safeguard. ;)


>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160119/70c1da5e/attachment-0001.html>