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