1466070 - Add clang-format as target of ./mach lint

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

A development which kept me partly busy in a flight between Paris & SF

I am not sure we want to do but at least, I would like to dump the code and start a discussion.

Currently, we have two ways to call clang-format:
1) ./mach clang-format => will run on a diff

2) ./mach clang-format -p <path> => will run on a path 

with the option -s to show the local changes or not.

With 
./mach lint -l clang-format
it will run only on whitelist directories (and not sure we have any in our code base?)
We can show the full diff at the end but I am not sure it makes sense to use the mozlint mechanism to show warnings/errors.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - WIP - Add clang-format as target of ./mach lint - WIP

Review commit: https://reviewboard.mozilla.org/r/248428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248428/

Comment 2

[Andrew Halberstadt [:ahal]]

I would be open to something like this, it's kind of similar to a patch I wrote almost a year ago for rustfmt in bug 1369792. I never ended up landing that because the fit was a little awkward (plus rustfmt wasn't stable yet). With rustfmt, there were seprate diffs for each issue, and I was able to stuff them into mozlint's 'source' attribute. I needed to modify the mozlint formatters to actually display this attribute though.

Do you have an example of what the output looks like? I think treating formatters like a linter is fine, though it could be contentious.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Lot of work for a small gain, closing for now

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Might be easier with the new option (--dry-run)
https://prereleases.llvm.org/10.0.0/rc3/tools/clang/docs/ReleaseNotes.html#clang-format

I will have a look

Comment 6

[Andrew Halberstadt [:ahal]]

From a high level, I think it would be nice if we kept formatting and linting as two separate commands. See my idea in bug 1511122 (and quick prototype patch in there).

That said, we already do run formatters in mach lint (at least rustfmt), so there's precedent for adding more if that's easier.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - Integrate clang-format into mozlint r?#linter-reviewers

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - mozlint/clang-format: create the new job r?#linter-reviewers

Depends on D69683

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - install clang-format-10 from apt.llvm.org r?#linter-reviewers

Depends on D69685

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - set the CLANGFORMAT variable to clang-format-10 (wip)

Depends on D69686

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1466070 - mozlint/pathutils: add some helper functions to get clang path r?#linter-reviewers

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11586e57bc1e
Integrate clang-format into mozlint r=linter-reviewers,ahal
https://hg.mozilla.org/integration/autoland/rev/bb752332e3f7
mozlint/clang-format: create the new job r=ahal

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/11586e57bc1e
https://hg.mozilla.org/mozilla-central/rev/bb752332e3f7