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