on 2021/7/19 下午11:59, Martin Sebor wrote: > On 7/19/21 12:20 AM, Kewen.Lin wrote: >> Hi, >> >> This patch follows Martin's suggestion here[1], to support >> range-based for loops for traversing loops, analogously to >> the patch for vec[2]. >> >> Bootstrapped and regtested on powerpc64le-linux-gnu P9, >> x86_64-redhat-linux and aarch64-linux-gnu, also >> bootstrapped on ppc64le P9 with bootstrap-O3 config. >> >> Any comments are appreciated. > > Thanks for this nice cleanup! Just a few suggestions: > > I would recommend against introducing new macros unless they > offer a significant advantage over alternatives (for the two > macros the patch adds I don't think they do). > > If improving const-correctness is one of our a goals > the loops_list iterator type would need to a corresponding > const_iterator type, and const overloads of the begin() > and end() member functions. > > Rather than introducing more instances of the loop_p typedef > I'd suggest to use loop *. It has at least two advantages: > it's clearer (it's obvious it refers to a pointer), and lends > itself more readily to making code const-correct by declaring > the control variable const: for (const class loop *loop: ...) > while avoiding the mistake of using const loop_p loop to > declare a pointer to a const loop. >
Thanks for the suggestions, Martin! Will update them in V2. With some experiments, I noticed that even provided const_iterator like: iterator begin () { return iterator (*this, 0); } + const_iterator + begin () const + { + return const_iterator (*this, 0); + } for (const class loop *loop: ...) will still use iterator instead of const_iterator pair. We have to make the code look like: const auto& const_loops = loops_list (...); for (const class loop *loop: const_loops) or template<typename T> constexpr const T &as_const(T &t) noexcept { return t; } for (const class loop *loop: as_const(loops_list...)) Does it look good to add below as_const along with loops_list in cfgloop.h? +/* Provide the functionality of std::as_const to support range-based for + to use const iterator. (We can't use std::as_const itself because it's + a C++17 feature.) */ +template <typename T> +constexpr const T & +as_const (T &t) noexcept +{ + return t; +} + BR, Kewen