Just to reduce number of uncategorized checks, since Clang-tidy has dedicated category for performance checks for awhile.
My build and regressions were OK on RHEL 6.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Makes sense. One comment re: documentation.
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
|---|---|---|
| 1 | I'd leave the old documentation file with a redirect to the new one. See examples in the cert module. But instead of calling the old check name 'alias', mention that it was renamed. We also don't need the old file to appear in the index, so it should be marked :orphan: and the add_new_check.py script should be updated to not add orphaned files to the list. | |
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
|---|---|---|
| 1 | I searched for orphan in documentation and didn't find any. Looks like documentation was always renamed in past without leaving traces to old names. I don't know documentation specific well enough, so will be good idea if it will be changed by more knowledgeable person then me. | |
Comment Actions
Makes sense. One comment re: documentation.
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
|---|---|---|
| 1 | An example of a doc file redirecting to the new location is docs/clang-tidy.rst. Specifically, you need to put :orphan: as the first line and then add redirection directive. | |
| clang-tidy/misc/CMakeLists.txt | ||
|---|---|---|
| 10 | What about MoveConstantArgumentCheck.cpp? | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
| 58 | 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.) | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
|---|---|---|
| 58 | 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. | |
| docs/clang-tidy/checks/misc-inefficient-algorithm.rst | ||
| 4 | We need to change the add_new_check.py script to exclude obsolete check names from the list (it could exclude all files marked :orphan:). Tell me, if you need help with this. | |
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
| 3 | After reading this check name a few times, I found it too generic (one may think that this is a generic algorithm-level code profiler ;). I think, we need to rename it to performance-inefficient-lookup-algorithm or performance-inefficient-search-algorithm, since we're changing the name anyway. | |
| 4 | Please make the underlining the same length as the line above. | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
|---|---|---|
| 58 | But we certainly need to mention the rename in the release notes for 3.8. | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
|---|---|---|
| 58 |
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). :-( | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
|---|---|---|
| 58 |
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. | |
| clang-tidy/misc/MiscTidyModule.cpp | ||
|---|---|---|
| 58 | (volunteers to implement this? ;)) | |
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
|---|---|---|
| 3 | I think will be better to keep generic name, since other algorithms could be added later. | |
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
|---|---|---|
| 3 | That is an interesting question whether it is better to have more general check names and make checkers do more stuff or have more specific names and split functionality between more checks. It would be awesome to have a policy on that. A good benchmark whether two checks should be implemented by the same checker is to think about whether there are cases when the user might enable only one of the checks, not both. | |
Comment Actions
Please rebase the patch and add full context to the diffs (see http://llvm.org/docs/Phabricator.html).
| docs/clang-tidy/checks/misc-inefficient-algorithm.rst | ||
|---|---|---|
| 4 | No need to do this, since the script already implements an alternative solution. | |
| docs/clang-tidy/checks/performance-inefficient-algorithm.rst | ||
| 3 |
In this case the check detects one consistent set of patterns (non-member lookup algorithms used on containers that implement better alternatives). I don't think we'll want to add something unrelated to this check, so I'd better make the name more specific and create another check, when we have a different set of patterns. | |
What about MoveConstantArgumentCheck.cpp?