mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Date Fri, 28 Sep 2018 09:12:18 GMT
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());
}

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message