mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject Re: clang-tidy and static code analysis
Date Mon, 27 Aug 2018 08:02:57 GMT
Great suggestion Philip, and thanks for the reference.  I've run a few
sanitizers locally but didn't see any output that concerned me. Absolutely
on my todo list to get them integrated into CI (unless someone beats me to
it).

On Sun, Aug 26, 2018 at 1:54 AM Marco de Abreu
<marco.g.abreu@googlemail.com.invalid> wrote:

> Great idea, thanks a lot Kellen. I had a look at the results and I really
> like that they are clearly actionable and reveal issues that somebody
> wouldn't have caught on the first sight.
>
> My personal favorite is the branch anomaly detection which finds execution
> paths that would result in a function returning garbage data if a certain
> path is being taken. I'm looking forward to more results!
>
> Best regards,
> Marco
>
> Philip Cho <chohyu01@cs.washington.edu> schrieb am Sa., 25. Aug. 2018,
> 21:48:
>
> > +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