-0.5 if keeping it as a warning.
On Thu, Oct 4, 2018 at 1:23 AM kellen sunderland < kellen.sunderl...@gmail.com> wrote: > I agree that active C++ developers should be the ones making these > choices. The downside to that is that many of these people are already > pretty busy. To make the best use possible of their time it would probably > make sense to create a concise doc with proposed style changes and ETAs > rather than focusing on a single change (modernized range loops). We've > got the infrastructure in place now to support any decisions made, and I > think it's in the best interest of the project as it will (1) unify coding > style to help readability and (2) make the lifes easier for reviewers which > are a critical resource on this project. > > I've update my PR such that it still modernizes all existing loops, but it > will now only issue a warning in the case that someone commits an older or > slower version of a loop. Of course as we've mentioned a few times this > only applies to cases where loops can be drop-in replaces with range > loops. I.e. no loops indexes are actively used, etc. > > Would you be alright with merging this change with warnings instead of > errors for the time being Chris? > > On Wed, Oct 3, 2018 at 7:20 PM Pedro Larroy <pedro.larroy.li...@gmail.com> > wrote: > > > +1 > > > > @Chris: do you have data on the performance difference? As far as I know > > there's a "rewrite rule" like the one between lambdas and C++ functors, > so > > performance should be very well defined, but maybe there's something that > > you are point out that we are missing. > > > > Having a consistent and concise code base is beneficial, I think what > > Kellen is advocating is to use range loops whenever possible, not > > prescribing its usage on every case, if you have to iterate backwards > there > > are other ways such as backwards iterator or others. > > > > On Fri, Sep 28, 2018 at 6:54 AM Chris Olivier <cjolivie...@apache.org> > > wrote: > > > > > -1 > > > > > > Range loops aren’t 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 you remove it > > from > > > the list at the bottom of the loop.... Seems like a rule for the sake > of > > > having a rule. > > > > > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland < > > > kellen.sunderl...@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 are 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 change. There > would > > > be > > > > no need for reviewers to have to manually provide feedback 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/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E > > > > > > > > ------------------------- > > > > Examples: > > > > for (auto axis_iter = param.axis.begin() ; axis_iter!= > > param.axis.end(); > > > > ++axis_iter) { > > > > CHECK_LT(*axis_iter, static_cast<int>(ishape.ndim())); > > > > stride_[reverse_index] = ishape[*axis_iter]; > > > > ... > > > > --> > > > > for (int axis : param.axis) { > > > > CHECK_LT(axis, static_cast<int>(ishape.ndim())); > > > > stride_[reverse_index] = ishape[axis]; > > > > ... > > > > -------------------------- > > > > for (size_t i = 0; i < in_array.size(); i++) { > > > > auto &nd = in_array[i]; > > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, > nd.dtype()); > > > > } > > > > --> > > > > for (auto & nd : in_array) { > > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, > nd.dtype()); > > > > } > > > > > > > > > >
