+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 <[email protected]> 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 <[email protected] > > > 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 > > >
