On Tue, 20 Jul 2021 at 09:58, Kewen.Lin <li...@linux.ibm.com> wrote:
>
> 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
The noexcept is not needed because GCC is built -fno-exceptions. For
consistency with all the other code that doesn't use noexcept, it
should probably not be there.
> +{
> + return t;
> +}
> +
That's one option. Another option (which could coexist with as_const)
is to add cbegin() and cend() members, which are not overloaded for
const and non-const, and so always return a const_iterator:
const_iterator cbegin () const { return const_iterator (*this, 0); }
iterator begin () const { return cbegin(); }
And similarly for `end () const` and `cend () const`.
