From dev-return-3975-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Sat Aug 25 21:48:55 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 4196C180654 for ; Sat, 25 Aug 2018 21:48:55 +0200 (CEST) Received: (qmail 4233 invoked by uid 500); 25 Aug 2018 19:48:54 -0000 Mailing-List: contact dev-help@mxnet.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mxnet.incubator.apache.org Delivered-To: mailing list dev@mxnet.incubator.apache.org Received: (qmail 4221 invoked by uid 99); 25 Aug 2018 19:48:53 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 25 Aug 2018 19:48:53 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 53120180414 for ; Sat, 25 Aug 2018 19:48:53 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.899 X-Spam-Level: * X-Spam-Status: No, score=1.899 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001] autolearn=disabled Authentication-Results: spamd3-us-west.apache.org (amavisd-new); dkim=pass (1024-bit key) header.d=cs.washington.edu Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id lzc_EsECKgal for ; Sat, 25 Aug 2018 19:48:49 +0000 (UTC) Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id E78E65F175 for ; Sat, 25 Aug 2018 19:48:48 +0000 (UTC) Received: by mail-wr1-f48.google.com with SMTP id u12-v6so8865957wrr.4 for ; Sat, 25 Aug 2018 12:48:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cs.washington.edu; s=goo201206; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=vNRp0PwAdMnmb6ip9aODyQdJ2MyY7Boc0hrGgllfh+c=; b=hbFuAhGgKiTfG7DpYe6Q5wdasbTvJH/HWBBzY2KdQqKcZ8Fi8ouZO5DUfc6pjfUwlL 4BIVQxPsKQpHD6MgXbfs/t+BXCCvJ42QY/qOMMzBcwbGrM0laY0m0VetjARr6uQir3ZO 682hs1IYLcsZj22iP+Iama4dXljgtCwyh73Rw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=vNRp0PwAdMnmb6ip9aODyQdJ2MyY7Boc0hrGgllfh+c=; b=aS6jlDzFTfiywRDxMoQwv/pC2gMjiONDZFebv1FNm9J3oFP33sLN8ncdcVPjOFcJbl 0dCrM4Rv5eCGHysSKyHIACbShC7ZL80EdpxwqgH3tQaFNF8Qhe+HKbTPnaeDwEP/XIgW GBOmOLgHibrnQFt4GcbOkKFvefSuhj4mHnzICj/gvgCRJHf6Qaa9szoZz7rkejM71ZoX Ow5OL/Dw+M+1Mp4dx6YNzTmyf4/hsGXbwku602wk229Ugb5z97sPpvXsRbpt1qDHwfAm elVAQDWBxsRJsbqeocgnYkXPgMPwZ9aPhoZuzRlBCYYl9Vc93VdPYe8nocew5Gf5RM0m GKSQ== X-Gm-Message-State: APzg51DaNF0UBaBZRwjfa7cSynSka6WkJxGpIxDAGCI00JhaITp2A7Su 4L4bz6oKNLQG4CTQGBD8q63MlIZ8z5ifUJnl/r5pbJtD X-Google-Smtp-Source: ANB0VdZvGmM8Mlk0PqxHLeKS6k7lxQskW9FffrrzXAEZRdBc5ygHgedUF8tXHLMo6oPGbCEho8O7Diyy7EtXOd2vbtU= X-Received: by 2002:adf:aa06:: with SMTP id p6-v6mr4510477wrd.56.1535226521362; Sat, 25 Aug 2018 12:48:41 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Philip Cho Date: Sat, 25 Aug 2018 12:48:30 -0700 Message-ID: Subject: Re: clang-tidy and static code analysis To: dev@mxnet.incubator.apache.org Content-Type: multipart/alternative; boundary="00000000000074b7ef057447c77c" --00000000000074b7ef057447c77c Content-Type: text/plain; charset="UTF-8" +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 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 > > 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 > > > --00000000000074b7ef057447c77c--