You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mxnet.apache.org by kellen sunderland <ke...@gmail.com> on 2018/09/28 09:12:18 UTC
[DISCUSS] Use modernized C++11 range loops uniformly throughout the project
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());
}
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@gmail.com>.
-0.5 if keeping it as a warning.
On Thu, Oct 4, 2018 at 1:23 AM kellen sunderland <
kellen.sunderland@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 <pe...@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 <cj...@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.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 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());
> > > > }
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by kellen sunderland <ke...@gmail.com>.
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 <pe...@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 <cj...@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.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 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());
> > > }
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Pedro Larroy <pe...@gmail.com>.
+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 <cj...@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.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 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());
> > }
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Lin Yuan <ap...@gmail.com>.
+1
Using range-based for-loop whenever possible improves code readability and
makes code less prone to human error.
I did some preliminary research on Google and did not find any complaint
about its performance drawback. Here is one piece from StackOverflow for
reference:
https://stackoverflow.com/questions/10821756/is-the-ranged-based-for-loop-beneficial-to-performance
Lin
On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
kellen.sunderland@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 <cj...@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.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 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());
> > > }
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@gmail.com>.
unless you don’t think that’s reasonable...
On Sun, Sep 30, 2018 at 7:59 AM Chris Olivier <cj...@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.sunderland@gmail.com> wrote:
>
>> About 60 but they're all addressed In the ref PR.
>>
>> On Sun, Sep 30, 2018, 6:12 AM Chris Olivier <cj...@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 <mn...@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 <me...@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 <me...@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.sunderland@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 <mnnaveen@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 <
>> > > cjolivier01@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.sunderland@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 <
>> > > > >> cjolivier01@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.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
>> 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());
>> > > > >> > > > > > }
>> > > > >> > > > > >
>> > > > >> > > > >
>> > > > >> > > >
>> > > > >> > >
>> > > > >> >
>> > > > >>
>> > > > >
>> > > >
>> > >
>> >
>>
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@gmail.com>.
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.sunderland@gmail.com> wrote:
> About 60 but they're all addressed In the ref PR.
>
> On Sun, Sep 30, 2018, 6:12 AM Chris Olivier <cj...@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 <mn...@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 <me...@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 <me...@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.sunderland@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 <mn...@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 <
> > > cjolivier01@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.sunderland@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 <
> > > > >> cjolivier01@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.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
> 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());
> > > > >> > > > > > }
> > > > >> > > > > >
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by kellen sunderland <ke...@gmail.com>.
About 60 but they're all addressed In the ref PR.
On Sun, Sep 30, 2018, 6:12 AM Chris Olivier <cj...@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 <mn...@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 <me...@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 <me...@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.sunderland@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 <mn...@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 <
> > cjolivier01@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.sunderland@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 <
> > > >> cjolivier01@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.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 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());
> > > >> > > > > > }
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@gmail.com>.
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 <mn...@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 <me...@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 <me...@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.sunderland@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 <mn...@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 <
> cjolivier01@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.sunderland@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 <
> > >> cjolivier01@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.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 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());
> > >> > > > > > }
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Naveen Swamy <mn...@gmail.com>.
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 <me...@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 <me...@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.sunderland@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 <mn...@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 <cj...@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.sunderland@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 <
> >> cjolivier01@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.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 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());
> >> > > > > > }
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Anton Chernov <me...@gmail.com>.
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 <me...@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.sunderland@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 <mn...@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 <cj...@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.sunderland@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 <
>> cjolivier01@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.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 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());
>> > > > > > }
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Anton Chernov <me...@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.sunderland@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 <mn...@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 <cj...@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.sunderland@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 <
> cjolivier01@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.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 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());
> > > > > > }
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by kellen sunderland <ke...@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 <mn...@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 <cj...@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.sunderland@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 <cj...@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.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 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());
> > > > > }
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Naveen Swamy <mn...@gmail.com>.
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 <cj...@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.sunderland@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 <cj...@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.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 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());
> > > > }
> > > >
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@gmail.com>.
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.sunderland@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 <cj...@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.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 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());
> > > }
> > >
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by kellen sunderland <ke...@gmail.com>.
"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 <cj...@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.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 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());
> > }
> >
>
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout
the project
Posted by Chris Olivier <cj...@apache.org>.
-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.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 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());
> }
>