Closed Bug 1480480 Opened Last year Closed 9 months ago

All clang-tidy checks should be either published or have a valid explanation why not

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: janx, Assigned: andi)

Details

Attachments

(2 files)

In tools/clang-tidy/config.yaml[0], we list several clang-tidy checks that we run to analyze Firefox code (via `./mach static-analysis check`, and via `reviewbot` on MozReview and Phabricator).

Some of these checks have `published: no`. We should either change that to `published: yes`, or explain why we don't publish these checks (and ideally file a bug for that problem).

[0] https://hg.mozilla.org/mozilla-unified/file/tip/tools/clang-tidy/config.yaml
Please lets explain why we don't use them. Also this would help:

https://bugzilla.mozilla.org/show_bug.cgi?id=1479298
Attachment #8997092 - Flags: feedback?(bpostelnicu)
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> Please lets explain why we don't use them. Also this would help:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1479298

Thanks! Please feel free to steal or re-do my WIP patch. :)

Note, here is what auto-fixing with 'readability-redundant-smartptr-get' does (if you include all third party files):

https://github.com/jankeromnes/gecko-dev/commit/5663a0b2662211b4b33f284e7aab6f38545d955a

And I see on https://nextcairn.com/mozilla-sa/#/stats that we've detected it 127 in reviews in the past 7 days (but setting `publish: yes` doesn't mean we'll necessarily publish them, e.g. they'll still be silent if in third-party code or not in the developer's patch).
Sure I will take this, thanks for reporting it.
Assignee: nobody → bpostelnicu
Ping about this. :)

Also, while you're editing comments inside this file, I have a few nits about this line at the top:

> # It is used by 'mach static-analysis' and 'mozreview static-analysis bot'

Could you please remove the word "mozreview", and add links to the relevant code locations?

I.e.:
- https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py
- https://github.com/mozilla/release-services/blob/master/src/staticanalysis/bot/
Flags: needinfo?(bpostelnicu)
Attachment #8997092 - Flags: feedback?(bpostelnicu)
Jan your nits are very pertinent, will address them shortly.
Flags: needinfo?(bpostelnicu)
Flags: needinfo?(bpostelnicu)

Andi, could you refresh this patch?

Flags: needinfo?(bpostelnicu)
Attachment #9012867 - Attachment description: Bug 1480480 - Explain why some clang-tidy checkers are not published. r=janx → Bug 1480480 - Explain why some clang-tidy checkers are not published. r=sylvestre
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fece2ece13b
Explain why some clang-tidy checkers are not published. r=sylvestre
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

clearing need-info since we've pushed this to m-c.

Flags: needinfo?(bpostelnicu)
You need to log in before you can comment on or make changes to this bug.