C++ Style Checking in CI #316
Assignees
Labels
feature
New functionality, or change in existing functionality
Comments
aokellermann
added
the
feature
|
Have a look as well to github actions and clang-format: https://github.com/marketplace/actions/clang-format-lint |
Merged
maxitg
added a commit
that referenced
this issue
May 19, 2020
## Changes * This closes the C++ Style Refactor project. * Resolves #316. * Resolves #317. * Resolves #319. * Closes #357. * Reformats all C++ code according to [Google C++ Style Guide]. * Adds `.clang-format` file specifying the exceptions to the Google style guide. * Adds lint.sh script which prints a diff generated by `clang-format` (if any) and the errors generated by `cpplint`, and allows one to apply formatting in place with `./lint.sh -i`. * Adds Lint step to CI before Build. * Updates .gitbub/CONTRIBUTING.md with instructions on how to use lint.sh. * Adds the missing test file to the Xcode project. ## Comments * Linting in CI is not parallelized and is using the same Docker image because it will eventually include linting of the Wolfram Language code as well as described in #247. * Bin packing is now disallowed in addition to the existing style exceptions. * All `cpplint` errors are fixed as well, not just the formatting. * `#include <chrono>` is not allowed by `cpplint` for Chromium-specific reasons, so added `// NOLINT` there. * Renamed `SetTest.cpp` -> `Set_test.cpp` because the suffix appears to be hard-coded to `cpplint`. * Before the formatting, the code was mostly following this style (imperfectly): ``` --- Language: Cpp BasedOnStyle: Google ColumnLimit: 120 BinPackArguments: false BinPackParameters: false AllowShortFunctionsOnASingleLine: Empty IndentWidth: 4 NamespaceIndentation: All AccessModifierOffset: -4 FixNamespaceComments: false SpaceAfterTemplateKeyword: false BreakConstructorInitializers: AfterColon SpacesBeforeTrailingComments: 1 ... ``` ## Examples * Break formatting/code style somewhere in the code, and run `./lint.sh`. For example, we can remove `explicit` on line 22 of Match.cpp, and add a spurious space on the line below: ``` > ./lint.sh --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * We can run `./lint.sh -i` to fix the space automatically: ``` > ./lint.sh -i --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * The output is the same, however, if we run `./lint.sh` again, there will be no diff: ``` > ./lint.sh libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * Finally, if we put explicit back in, there will be no output, which means everything is ok: ``` > ./lint.sh ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The problem
It would be best if we could automate the checking of C++ code style in CI. This would allow PR reviewers to spend less time manually linting.
Possible solution
Add a cpplint step to automated CI test. Linter script must be installed on CI Docker image as well.
The text was updated successfully, but these errors were encountered: