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."
~Aaron
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
