mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philip Cho <chohy...@cs.washington.edu>
Subject Re: clang-tidy and static code analysis
Date Sat, 25 Aug 2018 19:48:30 GMT
+1 as well.

As a related idea, let's also consider adding sanitizer tests, which
detects leaks and memory errors at runtime. Overhead is a lot lower than
other methods such as valgrind.
For an example, see the sanitizer tests in XGBoost:
https://github.com/dmlc/xgboost/pull/3557
https://github.com/dmlc/xgboost/pull/3525

Hyunsu Cho.

On Sat, Aug 25, 2018, 11:47 AM Hagay Lupesko <lupesko@gmail.com> wrote:

> 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