<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 19, 2016 at 4:28 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Jan 19, 2016 at 10:20 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
> alexfh added inline comments.<br>
><br>
> ================<br>
> Comment at: clang-tidy/misc/MiscTidyModule.cpp:58<br>
> @@ -57,3 +56,1 @@<br>
> -    CheckFactories.registerCheck<InefficientAlgorithmCheck>(<br>
> -        "misc-inefficient-algorithm");<br>
>      CheckFactories.registerCheck<MacroParenthesesCheck>(<br>
> ----------------<br>
> aaron.ballman wrote:<br>
>> alexfh wrote:<br>
>> > alexfh wrote:<br>
>> > > aaron.ballman wrote:<br>
>> > > > 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?<br>
>> > > ><br>
>> > > > (Perhaps a follow-up patch to allow deprecation of check names so that users are given guidance would make sense.)<br>
>> > > 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.<br>
>> > But we certainly need to mention the rename in the release notes for 3.8.<br>
>> > 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.<br>
>><br>
>> 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.<br>
>><br>
>> Oye. I just tested this out and the results were...surprisingly unhelpful.<br>
>> ```<br>
>> e:\llvm\2015>clang-tidy -checks=misc-hahahaha-nope E:\Desktop\test.cpp --<br>
>> e:\llvm\2015><br>
>> ```<br>
>> 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). :-(<br>
>> 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?<br>
><br>
> 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.<br>
<br>
</div></div>The user wrote something and likely assumed it had an effect when it<br>
doesn't have one -- that doesn't seem like intuitive (or particularly<br>
useful) behavior as far as the user is concerned. Typos are easy<br>
mistakes to make ("is inefficient spelled 'inefficient' or<br>
'inefficeint', that whole i before e thing is so tricky"), and the<br>
problem here is that it can be hard for a user to detect when they've<br>
messed up the filter because it's impossible to tell the difference<br>
between "check never ran" and "my code is perfect."<br></blockquote><div><br></div><div>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. ;)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>