Now that we've upgraded to clang-tidy 7.0, please enable a few new checkers

RESOLVED FIXED in Firefox 64

Status

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: janx, Assigned: janx)

Tracking

Trunk
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

In /tools/clang-tidy/config.yaml, we have two comments about enabling the following checks (once we have clang-tidy 6+):

- performance-implicit-conversion-in-loop
- readability-static-accessed-through-instance

Additionally, clang-tidy 7 also has the following additional checkers, which we should probably enable as well:

- clang-analyzer-security.insecureAPI.bcmp
- clang-analyzer-security.insecureAPI.bcopy
- clang-analyzer-security.insecureAPI.bzero
- performance-inefficient-algorithm
- performance-move-const-arg
- performance-move-constructor-init
- performance-noexcept-move-constructor

(Note: We used to enable `clang-analyzer-security.*` and `performance-*` before, which would have included these.)
Let's do a ./mach static-analysis autotest with this and see the results?
Or even better let's run it on try.
Attachment #9004187 - Attachment description: Bug 1466427 - Enable two new clang-tidy 7.0 checks. r=andi → Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi
New revision with suggested additional checkers + tests.

> Let's do a ./mach static-analysis autotest with this and see the results?

./mach static-analysis autotest passes locally.

> Or even better let's run it on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cbde9ce4f74a86d3d66e69dbe8929a4ac80d466
Green!

Not sure how to set `r?` again on Phabricator, but Andi please take another look.
Flags: needinfo?(bpostelnicu)
Comment on attachment 9004187 [details]
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi

Andi-Bogdan Postelnicu [:andi] has approved the revision.
Attachment #9004187 - Flags: review+
Flags: needinfo?(bpostelnicu)
Thanks Andi!

Adding keyword "leave-open" in case you find more 7.0 checkers that should be enabled here, but feel free to remove the keyword / open another bug if you'd like.

Meanwhile, sheriffs, please land this patch (it can simply be landed via Lando[0], but I chose not to have Level 3 commit access, so I can't click on the button).

[0] https://lando.services.mozilla.com/revisions/D4210/10911/
Bugs are cheap, please open a new bug :)
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9d93a20e6d6
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi
Busted on Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=196199668&repo=autoland&lineNumber=1698

"14:52:15     INFO -  ERROR: clang-tidy auto-test failed for checker performance-inefficient-algorithm Expected: [["warning", "this STL algorithm call should be replaced with a container method", "performance-inefficient-algorithm"]] Got: []"

Also the bug number in the commit message is wrong.
Backed out in bug 1466427 comment 48.
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/72c2fc2d0ab1
Backed out changeset f9d93a20e6d6 for Sa failures error: clang-tidy auto-test failed for checker performance-inefficient-algorithm CLOSED TREE
It seems that the checker "performance-inefficient-algorithm" finds defects in the test file "tools/clang-tidy/test/performance-inefficient-algorithm.cpp" on Linux, but not on Windows.

Andi or Sylvestre, do you know if this is expected? (This STL best practice checker doesn't strike me as platform-specific.)
Flags: needinfo?(sledru)
Flags: needinfo?(bpostelnicu)
dunno, sorry!
I would enable just on Linux for now and report a new bug to enable it on windows too once Andi is back!
Flags: needinfo?(sledru)
Jan please restrict it on Linux and open a new bug and directly assign it to me. We’ve had some similar issues in the past due to prototyping.
Flags: needinfo?(bpostelnicu)
Thanks a lot for your (too) quick replies! I'll disable this test on Windows and file a follow-up as requested.
Restricted "performance-inefficient-algorithm" checker to non-Windows platforms.

New Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e073c05195cb35f4553d77f210e8537c6480cd3b
Green! Sheriffs, please re-land this patch: https://lando.services.mozilla.com/revisions/D4210/12605/
Keywords: checkin-needed
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/146b7cab1762
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/146b7cab1762
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9004187 [details]
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi

This is a fake uplift request to test Uplift Bot (see bug 1492455 comment 2). Please ignore.
[Feature/Bug causing the regression]: TEST
[User impact if declined]: TEST
[Is this code covered by automated tests?]: TEST
[Has the fix been verified in Nightly?]: TEST
[Needs manual test from QE? If yes, steps to reproduce]: TEST
[List of other uplifts needed for the feature/fix]: TEST
[Is the change risky?]: TEST
[Why is the change risky/not risky?]: TEST
[String changes made/needed]: TEST
Attachment #9004187 - Flags: approval-mozilla-release?
Comment on attachment 9004187 [details]
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi

(Cancelled manually.)
Attachment #9004187 - Flags: approval-mozilla-release?
Comment on attachment 9004187 [details]
Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi

Approval Request Comment
[Feature/Bug causing the regression]: TEST, PLEASE IGNORE
[User impact if declined]: TEST
[Is this code covered by automated tests?]: TEST
[Has the fix been verified in Nightly?]: TEST
[Needs manual test from QE? If yes, steps to reproduce]: TEST
[List of other uplifts needed for the feature/fix]: TEST
[Is the change risky?]: TEST
[Why is the change risky/not risky?]:TEST
[String changes made/needed]: TEST
Attachment #9004187 - Flags: approval-mozilla-release?
Sorry, Uplift Bot failed to merge these patches onto mozilla-b'release':

* Merge failed for commit b'146b7cab1762' (parent da3cd3cea8c0):
grafting 485192:146b7cab1762 "Bug 1486410 - Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi"
merging tools/clang-tidy/config.yaml
merging tools/clang-tidy/test/structures.h
 warning: conflicts while merging tools/clang-tidy/config.yaml! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')


Please ensure your patches apply cleanly, then request uplift again.
Attachment #9004187 - Flags: approval-mozilla-release?
Sorry, Uplift Bot failed to merge these patches onto mozilla-release:

* Merge failed for commit b'146b7cab1762' (parent da3cd3cea8c0):
grafting 485192:146b7cab1762 "Bug 1486410 - Bug 1466427 - Enable new clang-tidy 7.0 checks. r=andi"
merging tools/clang-tidy/config.yaml
merging tools/clang-tidy/test/structures.h
 warning: conflicts while merging tools/clang-tidy/config.yaml! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')


Please ensure your patches apply cleanly, then request uplift again.
Flags: needinfo?(janx)
Flags: needinfo?(janx)
You need to log in before you can comment on or make changes to this bug.