Skip to content

Conversation

@dbaarda
Copy link
Member

@dbaarda dbaarda commented Aug 18, 2021

This adds clang-tidy and iwyu make targets for linting code, and adds a lint.yml githup action to run them.

Fixes #215 and replaces #222. This takes ideas from rizsotto's #222 pull request and changes them in the following ways;

  1. it drops the analyze-build checks which research indicates are a subset of clang-tidy.
  2. it sets CMAKE_EXPORT_COMPILE_COMMANDS ON in CMakeLists.txt so it doesn't need to be set at the cmdline.
  3. it adds clang-tidy and iwyu make targets, so that they can easily be executed at the cmdline too.
  4. it puts all the lint checks into a single github action, with each check as a separate step. This should be more efficient, and just as easy or easier to check the results.
  5. Adds a iwyu-fix make target for automatically fixing headers using fix_include. This skips fileutil.c which confuses these tools by conditionally including all the available file related headers to try and find working functions for different platforms.
  6. Fixes all warnings found by clang-tidy. There was actually not many.
  7. Fixes all the include warnings found by iwyu except for fileutil.c. This included making the include guards consistently named, adding include guards to all the headers that didn't have them, and fixing all the includes.
  8. Updates CONTRIBUTING.md to add a Requirements section listing dependencies and support tools to install, and how to use the new make targets.
  9. Update NEWS.md with changes added so far.

dbaarda added 16 commits August 18, 2021 23:00
This is a "linter" that checks code for errors, taken from PR librsync#222 by
riszotto.
It seems the ubuntu iwyu package has a bug where it's missing a dependency
containing the headers it needs.
This is to try and figure out why iwyu is still not finding standard headers.
This is the missing dependency needed by iwyu.
The clang-analyzer-valist.Uninitialized check has a bug that gives
false-positives. See https://bugs.llvm.org/show_bug.cgi?id=41311.
Fixed stats.c and sumset_test.c for minor clang warnings about unused values.
Always use FILENAME_H as the include guard name for filename.h. Also make
commens on include guards consistent.
This was mostly done by piping iwyu_tool output to `fix_include
--noblank_lines` with the exception of hashtable.h and its unittest which
required manual tweaking, plus a few manual tweaks in some places.

For fileutil.c it was excluded and only manually tweaked a tiny bit. It has
conditional includes based on config.h settings which seems to confuse
iwyu_tool and fix_includes to the point where it recommended removing several
of them which wouldn't compile any more.
This uses iwyu_tool output fed to `fix_include --noblank_lines` and excludes
fileutil.c which gets messed up by this.
Restructure the document into headings that better represent the development
processes. Update and expand the entries in each section to include new make
targets and github actions.
@dbaarda dbaarda changed the title Add clang-tidy and iwyu make targets and a lint.yml github action to run them. Add lint.yml github action and clang-tidy and iwyu make targets. Aug 20, 2021
@dbaarda dbaarda changed the title Add lint.yml github action and clang-tidy and iwyu make targets. Add lint.yml github action and make targets for clang-tidy and iwyu. Aug 20, 2021
@dbaarda dbaarda changed the title Add lint.yml github action and make targets for clang-tidy and iwyu. Add github action and make targets for clang-tidy and iwyu. Aug 20, 2021
@dbaarda
Copy link
Member Author

dbaarda commented Aug 20, 2021

I'm happy with this now, and I think I've dotted all the i's and crossed all the t's.

I'm going to merge this now.

@dbaarda dbaarda merged commit 0b4c21d into librsync:master Aug 20, 2021
@dbaarda dbaarda deleted the dev/actions4 branch August 20, 2021 06:08
@dbaarda dbaarda mentioned this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate from Travis to GitHub Actions.

1 participant