mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hagay Lupesko <lupe...@gmail.com>
Subject Re: clang-tidy and static code analysis
Date Sat, 25 Aug 2018 18:47:26 GMT
I really like this proposal.
It will help improve the quality of MXNet native code, and maintain a
uniform high bar.
An extra 5 mins of build time seems reasonable.

+1


On Sat, Aug 25, 2018, 07:02 kellen sunderland <kellen.sunderland@gmail.com>
wrote:

> 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