unless you don’t think that’s reasonable... On Sun, Sep 30, 2018 at 7:59 AM Chris Olivier <cjolivie...@gmail.com> wrote:
> If you get three +1’s from the top 6 contributors of C++ code (by volume), > I’ll switch to -0, since the ones committing the most C++ code will be the > most impacted and probably it should be their decision, imho. > > On Sun, Sep 30, 2018 at 12:28 AM kellen sunderland < > kellen.sunderl...@gmail.com> wrote: > >> About 60 but they're all addressed In the ref PR. >> >> On Sun, Sep 30, 2018, 6:12 AM Chris Olivier <cjolivie...@gmail.com> >> 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 <mnnav...@gmail.com> >> 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 <mecher...@gmail.com> >> > wrote: >> > > >> > > > And if you want a more authoritative opinion on that check out what >> the >> > > C++ >> > > > core guidelines are saying [1]: >> > > > >> > > > > ES.71: Prefer a range-for-statement to a for-statement when there >> is >> > a >> > > > choice >> > > > > Reason >> > > > > Readability. Error prevention. Efficiency. >> > > > >> > > > Best regards >> > > > Anton >> > > > >> > > > [1] >> > > > >> > > > >> > > >> > >> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-for-range >> > > > >> > > > >> > > > сб, 29 сент. 2018 г. в 16:13, Anton Chernov <mecher...@gmail.com>: >> > > > >> > > > > +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 >> be >> > > 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 >> further >> > > > > changes and are more likely to accept them, evolving your >> programming >> > > > 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 = foo(); auto& x : thing.items()) { /* ... */ } // OK >> > > > > >> > > > > // since C++11 >> > > > > struct cow_string { /* ... */ }; >> > > > > // a copy-on-write string cow_string str = /* ... */; >> > > > > // 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 >> > > > > >> > > > > сб, 29 сент. 2018 г. в 13:15, kellen sunderland < >> > > > > kellen.sunderl...@gmail.com>: >> > > > > >> > > > >> It's more readable because it's concise and it's consistent for >> many >> > > > 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 >> loop >> > > > >> 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 =0 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 <mnnav...@gmail.com >> > >> > > > wrote: >> > > > >> >> > > > >> > Kellen, >> > > > >> > >> > > > >> > Could you please explain why you think range loops are better >> and >> > > 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 >> should >> > > > write >> > > > >> > C++ code for MXNet. >> > > > >> > >> > > > >> > -1(open to change based on new information) >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier < >> > > cjolivie...@gmail.com> >> > > > >> > wrote: >> > > > >> > >> > > > >> > > ok then, my vote is still -1, however, because it’s just >> adding >> > > > >> needless >> > > > >> > > friction for developers imho. >> > > > >> > > >> > > > >> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland < >> > > > >> > > kellen.sunderl...@gmail.com> wrote: >> > > > >> > > >> > > > >> > > > "Range loops aren’t 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 >> 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." >> > > > >> > > > >> > > > >> > > > 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 >> changes >> > > 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.html >> > > > >> > > > 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 >> to >> > > > 'safe' >> > > > >> > > which >> > > > >> > > > would further reduce the number of loops the check would >> apply >> > > to. >> > > > >> > > > >> > > > >> > > > -Kellen >> > > > >> > > > >> > > > >> > > > On Fri, Sep 28, 2018 at 3:54 PM 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()); >> > > > >> > > > > > } >> > > > >> > > > > > >> > > > >> > > > > >> > > > >> > > > >> > > > >> > > >> > > > >> > >> > > > >> >> > > > > >> > > > >> > > >> > >> >
