From dev-return-4327-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Sun Sep 30 09:28:47 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 EF2A518064A for ; Sun, 30 Sep 2018 09:28:46 +0200 (CEST) Received: (qmail 92941 invoked by uid 500); 30 Sep 2018 07:28:45 -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 92929 invoked by uid 99); 30 Sep 2018 07:28:45 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 30 Sep 2018 07:28:45 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id A629A1A14E0 for ; Sun, 30 Sep 2018 07:28:44 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.889 X-Spam-Level: * X-Spam-Status: No, score=1.889 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, SPF_PASS=-0.001, T_DKIMWL_WL_MED=-0.01] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id PTkrQDr9ug4g for ; Sun, 30 Sep 2018 07:28:41 +0000 (UTC) Received: from mail-yw1-f42.google.com (mail-yw1-f42.google.com [209.85.161.42]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id CBE4B5F180 for ; Sun, 30 Sep 2018 07:28:40 +0000 (UTC) Received: by mail-yw1-f42.google.com with SMTP id k66-v6so4298318ywa.10 for ; Sun, 30 Sep 2018 00:28:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hK8IV9ceQ8af1vLqC5PnRL8nnlp+BiLVtvx9QreJ5Ns=; b=crYQd0xxbscVepA3CVQpH3/Bn3DqNiqgwek2QEMWSa0H5zkt2Ut5ZsY6wuHkuzvDqZ cvfDhm1uZclwYK719bYz/bOID/7bWq0nkgfp3klr3BbKe3nMCWHL622lWhBA9mk/vrnb UalpqTV9VcstovTRFnQbVOw9W4TV/cUSKTg8Ik/7a45ijr5guEUcW+1dMXpZiYQZBkpo 5wv1mNJ4k3lEMLEKW2lMx0ygmv0wBFJXLNDlryRsgPFtv90bnsmn9UtkBukdWwRh8AE7 MVkcXAHMEBSlWkMvDMn4RURUcOmLCQ5oTagqY6XcuACkp9RUu4METkH+nUf8ajfryAnr 6SbA== 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:cc; bh=hK8IV9ceQ8af1vLqC5PnRL8nnlp+BiLVtvx9QreJ5Ns=; b=i+g4zdSt1PZ8I5tpuSVpCVW4DZVhxleYz5jweBETGmSWJ6wHbAEpNWkON+ZWkRj9nA VJ0SkVeiATgSzYSsAPCdtuhqsq/Q3DF+oiAWEikodBca8eV47mWR4hxZe1WW8vxsw8Qz Tp71a6bx/onii3rIYPv4dMJC2IjZIOjHYCjHnEaUQuEEa/fv8ZbvSgeqSfHg7Y/iCKPO ujKGR6NPgn7eJ+DcGoSWREWla8LoDMmnCA5j2EvtRuROrdp1ccpe21nl/l0idNHF/Hkf s7o3VXjTfUl/QjeTWGsJJxPoXWHH+NpxMQagBeQehoAAHUWyxJGdhe31zRKn6mgqfDo3 ixCw== X-Gm-Message-State: ABuFfojdlYbM4UwizZD4kyq9E1yfeTqa57geDBZhG/gWId/rtAFYyJ2Z PwUw57Y6A8DPlmUzWSfIxaKKRdMo9P3K1wTwUNfEKw== X-Google-Smtp-Source: ACcGV61VpC9xnIsS9oMHsSItbFonX/hOSGZqz08Ph9vGdiuv7u6jnVA6NyQl+7t7xxUu2uSGcThcHx33MFGtKsmeuEE= X-Received: by 2002:a81:2a42:: with SMTP id q63-v6mr2814968ywq.91.1538292513505; Sun, 30 Sep 2018 00:28:33 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: kellen sunderland Date: Sun, 30 Sep 2018 09:28:21 +0200 Message-ID: Subject: Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project To: dev@mxnet.incubator.apache.org Cc: dev@mxnet.apache.org Content-Type: multipart/alternative; boundary="000000000000d405e6057711a24c" --000000000000d405e6057711a24c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable About 60 but they're all addressed In the ref PR. On Sun, Sep 30, 2018, 6:12 AM Chris Olivier wrote: > How many errors exist in the code base right now if it were to be enabled= ? > > On Sat, Sep 29, 2018 at 7:03 PM Naveen Swamy wrote: > > > Thanks Kellen & Anton, for your detailed explanation and links to > > advantages, appreciate it. > > changing my vote to *-0*, I suggest to show as warnings. > > > > On Sat, Sep 29, 2018 at 8:06 PM Anton Chernov > wrote: > > > > > And if you want a more authoritative opinion on that check out what t= he > > C++ > > > core guidelines are saying [1]: > > > > > > > ES.71: Prefer a range-for-statement to a for-statement when there i= s > a > > > choice > > > > Reason > > > > Readability. Error prevention. Efficiency. > > > > > > Best regards > > > Anton > > > > > > [1] > > > > > > > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines= .md#Res-for-range > > > > > > > > > =D1=81=D0=B1, 29 =D1=81=D0=B5=D0=BD=D1=82. 2018 =D0=B3. =D0=B2 16:13,= Anton Chernov : > > > > > > > +1 > > > > > > > > Maybe it's not necessary to enforce usage of range-based for, but I > > would > > > > highly encourage to to it due to already named advantages. If code > > would > > > be > > > > introduced using the old-style there could be a comment suggesting > the > > > new > > > > way. But why do the manual work and not leave that to the automated > > tool? > > > > > > > > And since it's already automated - wouldn't it be better to keep a > > > unified > > > > modern style? > > > > > > > > Just to make this a trend - C++ evolves quickly and this will not b= e > > only > > > > upgrade that would needed to be made. And the easier such upgrades > get > > > > accepted the easier in general is to upgrade the codebase. > > > > > > > > Soon the standard will get ranges and concepts and this will change > the > > > > way C++ applications get written significantly. It is a good habit = to > > be > > > > open for changes and keep up with the trends. By using the new > > > > possibilities the language can offer you prepare yourself for furth= er > > > > changes and are more likely to accept them, evolving your programmi= ng > > > style. > > > > > > > > Take a look at a new examples on modern usages (taken from [1]): > > > > > > > > // since C++17 > > > > for (auto&& [first,second] : mymap) { > > > > // use first and second > > > > } > > > > > > > > // since C++20 > > > > for (auto& x : foo().items()) { /* .. */ } // undefined behavior if > > foo() > > > > returns by value > > > > for (T thing =3D foo(); auto& x : thing.items()) { /* ... */ } // O= K > > > > > > > > // since C++11 > > > > struct cow_string { /* ... */ }; > > > > // a copy-on-write string cow_string str =3D /* ... */; > > > > // for(auto x : str) { /* ... */ } // may cause deep copy > > > > for(auto x : std::as_const(str)) { /* ... */ } > > > > > > > > Regarding performance: it's really easy to prove that generated > > assembly > > > > is not changing at all. There is a really handy tool for that [2]. > You > > > can > > > > check online the assembly for different language constructs and > > different > > > > compilers. > > > > > > > > Best regards, > > > > Anton > > > > > > > > [1] https://en.cppreference.com/w/cpp/language/range-for > > > > [2] https://gcc.godbolt.org > > > > > > > > =D1=81=D0=B1, 29 =D1=81=D0=B5=D0=BD=D1=82. 2018 =D0=B3. =D0=B2 13:1= 5, kellen sunderland < > > > > kellen.sunderland@gmail.com>: > > > > > > > >> It's more readable because it's concise and it's consistent for ma= ny > > > types > > > >> you're looping over (i.e. primitive arrays, stl iterators, etc all > > work > > > >> the > > > >> same way). It's also useful because it's consistent with other > > > >> programming > > > >> languages, making C++ codebases much easier to read for novice and > > > >> intermediate developers. IMO it also leads to better naming in lo= op > > > >> bodies > > > >> as the concise style means you're less likely to have important 1 > > letter > > > >> variable names describing loop elements (e.g. no int i =3D0 or it > ...). > > > >> More > > > >> motivation can be found in the cpp standards proposals for C++11 > > > >> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html > > and > > > >> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm. > > > >> > > > >> > > > >> > > > >> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy > > > wrote: > > > >> > > > >> > Kellen, > > > >> > > > > >> > Could you please explain why you think range loops are better an= d > > how > > > it > > > >> > improves readability? this is a relatively new feature, many of > > them > > > >> are > > > >> > used to the old syntax, shouldn't we leave it for the developers > to > > > >> choose > > > >> > the one that best suits the need and their familiarity. > > > >> > In general I support the notion of standardizing where necessary= , > > > >> enforcing > > > >> > rules on loops seems little bit like micro-managing how you shou= ld > > > write > > > >> > C++ code for MXNet. > > > >> > > > > >> > -1(open to change based on new information) > > > >> > > > > >> > > > > >> > > > > >> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier < > > cjolivier01@gmail.com> > > > >> > wrote: > > > >> > > > > >> > > ok then, my vote is still -1, however, because it=E2=80=99s ju= st adding > > > >> needless > > > >> > > friction for developers imho. > > > >> > > > > > >> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland < > > > >> > > kellen.sunderland@gmail.com> wrote: > > > >> > > > > > >> > > > "Range loops aren=E2=80=99t always the most performant way" = Do you > have > > an > > > >> > > example > > > >> > > > where there's a perf difference? > > > >> > > > > > > >> > > > "In addition, sometimes you want the index. Or maybe you wan= t > to > > > >> > iterate > > > >> > > > backwards, or not start from the first, etc. Maybe you want > the > > > >> > iterator > > > >> > > > because you remove it from the list at the bottom of the > > loop.... > > > >> Seems > > > >> > > > like a rule for the sake of having a rule." > > > >> > > > > > > >> > > > I should have been more clear about this point. If you're > using > > > the > > > >> > > index > > > >> > > > in the loop, doing reverse iteration, or not iterating from > > > >> > start-to-end > > > >> > > > this inspection is smart enough to realize it and will not > > suggest > > > >> > > > optimizing that type of loop. The loops that would be chang= es > > are > > > >> > _only_ > > > >> > > > the loops which are detected as equivalent to range-loops. > > > Examples > > > >> > can > > > >> > > be > > > >> > > > found here: > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.htm= l > > > >> > > > or you can look at what's been changed in the ref PR. I've > > > >> initially > > > >> > set > > > >> > > > our confidence level at 'reasonable' but we could also set t= o > > > 'safe' > > > >> > > which > > > >> > > > would further reduce the number of loops the check would app= ly > > to. > > > >> > > > > > > >> > > > -Kellen > > > >> > > > > > > >> > > > On Fri, Sep 28, 2018 at 3:54 PM Chris Olivier < > > > >> cjolivier01@apache.org> > > > >> > > > wrote: > > > >> > > > > > > >> > > > > -1 > > > >> > > > > > > > >> > > > > Range loops aren=E2=80=99t always the most performant way.= In > > addition, > > > >> > > sometimes > > > >> > > > > you want the index. Or maybe you want to iterate backwards= , > or > > > not > > > >> > > start > > > >> > > > > from the first, etc. Maybe you want the iterator because y= ou > > > >> remove > > > >> > it > > > >> > > > from > > > >> > > > > the list at the bottom of the loop.... Seems like a rule f= or > > the > > > >> sake > > > >> > > of > > > >> > > > > having a rule. > > > >> > > > > > > > >> > > > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland < > > > >> > > > > kellen.sunderland@gmail.com> wrote: > > > >> > > > > > > > >> > > > > > Hello MXNet devs, > > > >> > > > > > > > > >> > > > > > I'd like to discuss uniformly adopting C++11 range loops > in > > > the > > > >> > MXNet > > > >> > > > > > project. The benefits I see are: > > > >> > > > > > > > > >> > > > > > * Improved C++ readability (examples below). > > > >> > > > > > * Consistency with other languages. The range-loops ar= e > > > quite > > > >> > > similar > > > >> > > > > to > > > >> > > > > > loops almost all other programming languages. Given > we're a > > > >> > project > > > >> > > > that > > > >> > > > > > supports many languages this language consistency could = be > > > >> positive > > > >> > > for > > > >> > > > > our > > > >> > > > > > community. > > > >> > > > > > * Consistency within the same project. Currently > different > > > >> authors > > > >> > > > have > > > >> > > > > > different loops styles which hurts codebase readability. > > > >> > > > > > * Best available performance. There are often multiple > > ways > > > to > > > >> > > write > > > >> > > > > > loops in C++ with subtle differences in performance and > > memory > > > >> > usage > > > >> > > > > > between loop methods. Using range-loops ensures we get > the > > > best > > > >> > > > possible > > > >> > > > > > perf using an intuitive loop pattern. > > > >> > > > > > * Slightly lower chance for bugs / OOB accesses when > > dealing > > > >> with > > > >> > > > > indexing > > > >> > > > > > in an array for example. > > > >> > > > > > > > > >> > > > > > If we decide to enable this uniformly throughout the > project > > > we > > > >> can > > > >> > > > > enable > > > >> > > > > > this policy with a simple clang-tidy configuration chang= e. > > > >> There > > > >> > > would > > > >> > > > > be > > > >> > > > > > no need for reviewers to have to manually provide feedba= ck > > > when > > > >> > > someone > > > >> > > > > > uses an older C++ loops style. > > > >> > > > > > > > > >> > > > > > -Kellen > > > >> > > > > > > > > >> > > > > > Reference PR: > > > >> > https://github.com/apache/incubator-mxnet/pull/12356/ > > > >> > > > > > Previous clang-tidy discussion on the list: > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa= 2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E > > > >> > > > > > > > > >> > > > > > ------------------------- > > > >> > > > > > Examples: > > > >> > > > > > for (auto axis_iter =3D param.axis.begin() ; axis_iter!= =3D > > > >> > > > param.axis.end(); > > > >> > > > > > ++axis_iter) { > > > >> > > > > > CHECK_LT(*axis_iter, static_cast(ishape.ndim())= ); > > > >> > > > > > stride_[reverse_index] =3D ishape[*axis_iter]; > > > >> > > > > > ... > > > >> > > > > > --> > > > >> > > > > > for (int axis : param.axis) { > > > >> > > > > > CHECK_LT(axis, static_cast(ishape.ndim())); > > > >> > > > > > stride_[reverse_index] =3D ishape[axis]; > > > >> > > > > > ... > > > >> > > > > > -------------------------- > > > >> > > > > > for (size_t i =3D 0; i < in_array.size(); i++) { > > > >> > > > > > auto &nd =3D in_array[i]; > > > >> > > > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), tru= e, > > > >> > > nd.dtype()); > > > >> > > > > > } > > > >> > > > > > --> > > > >> > > > > > for (auto & nd : in_array) { > > > >> > > > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), tru= e, > > > >> > > nd.dtype()); > > > >> > > > > > } > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > --000000000000d405e6057711a24c--