On Tue, Jan 19, 2016 at 11:06 AM, Alexander Kornienko <ale...@google.com> wrote:
> On Tue, Jan 19, 2016 at 4:28 PM, Aaron Ballman <aaron.ball...@gmail.com>
> wrote:
>>
>> On Tue, Jan 19, 2016 at 10:20 AM, Alexander Kornienko <ale...@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. ;)
Hmm, if only we knew of someone, anyone at all, who was bothered by
this. Hmmm.. ;-)
I can tackle this when I get the chance. :-D
~Aaron
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
