mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject clang-tidy and static code analysis
Date Sat, 25 Aug 2018 14:02:25 GMT
Hello all,

Inspired by Vanadana, cclaus and the project members who setup the very
solid linting tools already in place for MXNet, I'm propose we enable
clang-tidy-6.0 in our CI (PR here:
https://github.com/apache/incubator-mxnet/pull/12282).  clang-tidy is
getting to be quite a high-quality, free, easy-to-use static analysis tool
for modern C++.  In my opinion it's a very useful extension of clang's
already great code warning system.  Adding it to MXNet might help us catch
a few errors (memory leaks, use-after-frees, etc.) and it could help us
keep our coding standards uniform between contributors.  It should also
help automate some of the work that is currently required of the PR
reviewers.

I'd suggest we initially enabled clang-tidy as a mostly informational CI
build step that will give many warnings, as in this sample run:
http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/PR-12282/runs/20/nodes/102/log/?start=0
(warnings at bottom of the build).  We'll be able to optionally fail the
build if certain rules are violated.  There's a complete list of rules
here:
https://releases.llvm.org/6.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html.
If anyone has a controversial rule they'd like to see enforced, feel free
to nominate rule in this thread.  Non-controversial (use your best
judgement) rules can be enabled with a workflow similar to what we already
do with pylint.  Choose a rule, make changes to the codebase such that that
rule will pass and add the rule to the .clang-tidy configuration file in
the root of the repo.  The current formatting of the file should make it
obvious which rules are enabled as warnings/failures.

I'd estimate once the PR is merged the build times for this task will be
pretty nominal (4-5 minutes).  Since the task is run in parallel, it should
have no impact on the PR total verification times.  I also think that
introducing a tool like this right after a release has been cut will be
convenient for developers.  It's a good time to introduce large fixup
changes, and it will give us lot of time to find any potential side effects
of modernization refactors.

What does everyone think?  Does it make sense to introduce clang-tidy and
gradually address or enforce rules as with cpplint / pylint / flake8?

-Kellen

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message