pocc/pre-commit-hooks

Why use diff instead of exit status for pass/fail?

zbeekman opened this issue · 5 comments

On macOS if I pass --check=* to clang-tidy, it flags a macro expanded from a system header. If I add the appropriate clang-tidy directive to ignore the error on that line, the pre-commit hook still fails:

clang-tidy...........................................................................Failed
hookid: clang-tidy

1080 warnings generated.
Suppressed 1090 warnings (1080 in non-user code, 10 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

To elaborate: Why all the fancy diff legwork? Why not just examine the exit status?

pocc commented

The reason is that clang-tidy does not reliably produce a non-zero exit status on warnings/errors.

This bash example demonstrates this:

cd /tmp
echo '[{"directory":"/tmp","command":"gcc a.c -o a.o","file":"/tmp/a.c"}]' > compile_commands.json
echo 'int main() { int i; return 10; }' > a.c
clang-tidy a.c --checks=*
> 2 warnings generated.
> a.c:1:28: warning: 10 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
> int main() { int i; return 10; }
>                                        ^
echo $?
> 0

Hmmmm interesting. But if you silence errors/warnings with comments then the pre-commit action is still flagged as a failure. I guess I'll close this issue. You'd think that they would at least have a flag to exit non-zero, or turn warnings into errors.

@pocc Do you know if this is true even when using:

 --warnings-as-errors=<string>  -
                                   Upgrades warnings to errors. Same format as
                                   '-checks'.
                                   This option's value is appended to the value of
                                   the 'WarningsAsErrors' option in .clang-tidy
                                   file, if any.

?

pocc commented

--warnings-as-errors could be a valid alternative for clang-tidy. If memory serves, oclint returns 0 even on error. Thus diffing provides a more reliable method of determining pass/fail across all tools. I agree that exit status should be relied upon.

If at some point, all of the linters currently in this repo have a consistent and reliable exit status, please reopen this issue.