1756032 - [Automated review] reviewbot's clang-tidy errors aren't actionable

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P1)

Description

[Robin Steuber (they/them) [:bytesized]]

Attachment: static_analysis_output

Phabricator URL: https://phabricator.services.mozilla.com/D138593

Reviewbot keeps complaining about my patch.

Code analysis found 3 defects in the diff 540553:

• 3 build errors found by clang-tidy

You can run this analysis locally with:

• ./mach static-analysis check --outgoing (C/C++)

---

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Running the analysis locally is very confusing. I attached the output to this bug report. I see a number of problems with it:

1. It seems to be trying to use color codes. But they aren't working properly, which makes the whole thing really, really hard to read. This is not an uncommon problem on Windows. It is possible to make them work right (I use them in my BASH prompt). But it also seems to be really easy to screw them up. I think that Bug 1417003 has some more details about this.
2. There appear to be 18 different summary lines ("X warnings and Y errors generated"). It's not clear which, if any of them, I should pay attention to.
3. Despite the --outgoing flag, many (maybe all?) of the warnings and errors listed are not in outgoing code. Some of them appear to be in files (but not lines) that were touched in this patch. Others appear to be in files that this patch had nothing to do with (ex: Span.h).
4. "Error while processing" appears in the output 19 times, but it's unclear to me what the error is.

Additionally, I feel like there are just a whole bunch of problems with reviewbot's comment:

1. It doesn't seem to line up at all with what the local analysis says.
2. Unlike other reviewbot comments I've gotten, it doesn't give me any idea what the errors are. It doesn't call them out individually. It doesn't link to a patch that fixes them. The only suggestion of what to do is to run the analysis locally, but it doesn't line up with that.
3. The "the code-review frontend" link is also unhelpful. It does mention 3 errors. Which does seem to line up with the "3 build errors" that reviewbot mentions. But none of them are in code touched by this patch. It seems to acknowledge this with the red X by the "In Patch" line. But it seems to be mad about them anyways? I don't really understand why.
4. The above points make the reviewbot comment completely non-actionable.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Andi [:andi]]

This is an issue in our static-analysis checker.

Comment 6

[Andi [:andi]]

Attachment: Bug 1756032 - for `clang-tidy` based analysis also activate `clang-plugin`.

By activating clang-plugin we make sure that all defines for static-analysis are
in place.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e6e57de6883
for `clang-tidy` based analysis also activate `clang-plugin`. r=firefox-static-analysis-reviewers,marco

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8e6e57de6883