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

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 19 07:07:24 PST 2016


aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:58
@@ -57,3 +56,1 @@
-    CheckFactories.registerCheck<InefficientAlgorithmCheck>(
-        "misc-inefficient-algorithm");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
----------------
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). :-(


http://reviews.llvm.org/D16248





More information about the cfe-commits mailing list