This linter checks that the number of arguments passed to each variadic format
string function matches the number of format specifiers in the format string.
Backport of core PR13705
Details
Details
- Reviewers
jasonbcox deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGf867af275c65: build: Add format string linter
rABCf867af275c65: build: Add format string linter
./test/lint/lint-format-strings.sh
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D2025
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4356 Build 6777: Bitcoin ABC Buildbot (legacy) Build 6776: arc lint + arc unit
Event Timeline
Comment Actions
➜ bitcoin-abc git:(arcpatch-D2025) ✗ ./test/lint/lint-format-strings.sh ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found ./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #5707) | #!/bin/bash |
Comment Actions
Running it through arc lint is a good idea. I propose to do this in another diff to keep the backport clean
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #5707) | What is the reason for this change ? Is env causing an issue on Mac ? |
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #5707) | |
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 1 ↗ | (On Diff #5707) | I read the article, and I think using env is the best solution for this specific script. |
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 35 ↗ | (On Diff #5707) | What does this do? And why is this script-to-run-a-script necessary? I would prefer not to have shell running python if possible. It errors out on FreeBSD variants as mapfile isn't included. |
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 35 ↗ | (On Diff #5707) | It builds an array from the input. mapfile (or readarray, this is equivalent) is part of bash, but is also built in sh, what shell are you using ? I agree that this shell script should be avoided, the idea is to end up calling the python script from arc lint. But I don't know how to do this (yet), and I prefer include it in a future diff to keep the backport consistent. |
Comment Actions
So I arch patched this and this is CLEARLY not working.
$ time ./test/lint/lint-format-strings.sh [...] real 0m1.811s user 0m1.928s sys 0m0.616s $ time arc lint OKAY No lint warnings. real 0m0.544s user 0m0.448s sys 0m0.094s
Comment Actions
The linter actually runs on a single file if called via the bash script ./test/lint/lint-format-strings.sh file_to_check.
This is required for arcanist integration. When it is called via arc lint every file from the diff is processed individually (you can run arc lint --trace to see the behavior).
This is different from the core version which checked all the files.
| .arclint | ||
|---|---|---|
| 23 ↗ | (On Diff #6396) | I thought that errors from libraries should not be checked as it may not be code we want to maintain. These will also require to manually add exceptions in the python script, and re-check every time the library gets updated. The clang-format linter also excludes these files. |
| test/lint/lint-format-strings.sh | ||
|---|---|---|
| 39 ↗ | (On Diff #6400) | Why the change ? If the script failed to run, it should not pretend it's all good. |
Comment Actions
Split in two diffs. This one is the backport of PR13705.
The arcanist integration will be made in another diff.