Martin Liška <mli...@suse.cz> writes:
> On 6/16/20 4:14 PM, Richard Sandiford wrote:
>> Martin Liška <mli...@suse.cz> writes:
>>> Hmm, sounds like a nice abstraction but I see 2 problems with that:
>>>
>>> 1) How can I define a constructor of the iterator_pair when I need
>>> something like:
>>> iterator_pair<const_iterator> (region_begin, region_end)?
>>
>> Not sure if I'm answering the right question, sorry, but I meant
>> something along the lines of:
>>
>> template<typename T>
>> struct iterator_pair
>> {
>> public:
>> iterator_pair (const T &begin, const T &end)
>> : m_begin (begin), m_end (end) {}
>>
>> T begin () const { return m_begin; }
>> T end () const { return m_end; }
>>
>> private:
>> T m_begin;
>> T m_end;
>> };
>>
>>> 2) rbegin function is more complex than begin function:
>>>
>>> reverse_iterator rbegin () const
>>> {
>>> reverse_iterator it = reverse_iterator (m_region_end);
>>> if (*it == NULL)
>>> return reverse_iterator (gsi_last_bb (m_region_end.bb));
>>> else
>>> return ++it;
>>> }
>>>
>>> const_iterator begin () const { return const_iterator
>>> (m_region_begin); }
>>>
>>> How can we abstract that?
>>
>> With the change above, we'd replace “rbegin” and “rend”
>> with a “reverse_region_stmts” function that returns an
>> “iterator_pair<const_reverse_iterator>”. Its “begin” iterator
>> would have the value that “rbegin” calculates above and its “end”
>> iterator would have the same value as the current patch's “rend”.
>
> All right, you provided a nice hints! There's updated version of the patch.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> [...]
> + gimple *operator* () const { return gsi_stmt (gsi); }
> +
> + bool
> + operator== (const const_iterator& other) const
I think the usual formatting in GCC (though not elsewhere) is to put the
space before rather than after the “&”, like for pointers. Same for the
rest of the patch.
> + {
> + return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> + }
> +
> + bool
> + operator!= (const const_iterator& other) const
> + {
> + return !(*this == other);
> + }
> +
> + gimple_stmt_iterator gsi;
> + };
> +
> + /* GIMPLE statement iterator going from region_end to region_begin. */
> +
> + struct reverse_const_iterator
The standard name for this abstraction is const_reverse_iterator rather
than reverse_const_iterator.
> + {
> + reverse_const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
> +
> + const reverse_const_iterator &
> + operator++ ()
> + {
> + gsi_prev (&gsi); return *this;
> + }
> +
> + gimple *operator* () const { return gsi_stmt (gsi); }
> +
> + bool
> + operator== (const reverse_const_iterator& other) const
> + {
> + return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> + }
> +
> + bool
> + operator!= (const reverse_const_iterator& other) const
> + {
> + return !(*this == other);
> + }
> +
> + gimple_stmt_iterator gsi;
> + };
> +
> + /* Returns iterator_pair for range-based loop. */
> +
> + iterator_pair<const_iterator>
> + region_stmts ()
> + {
> + return iterator_pair<const_iterator> (const_iterator (region_begin),
> + const_iterator (region_end));
The const_iterator(…)s aren't necessary here, just:
return iterator_pair<const_iterator> (region_begin, region_end);
is IMO more readable.
> + }
> +
> + /* Returns iterator_pair for range-based loop in a reverse order. */
> +
> + iterator_pair<reverse_const_iterator>
> + reverse_region_stmts ()
> + {
> + reverse_const_iterator begin = reverse_const_iterator (region_end);
> + if (*begin == NULL)
> + begin = reverse_const_iterator (gsi_last_bb (region_end.bb));
> + else
> + ++begin;
Similarly here we don't need the reverse_const_iterator(…)s
(= const_reverse_iterator(…)s after the change above). It can just be:
const_reverse_iterator begin = region_end;
if (*begin == NULL)
begin = gsi_last_bb (region_end.bb);
else
++begin;
> + reverse_const_iterator end = reverse_const_iterator (region_begin);
> +
> + return iterator_pair<reverse_const_iterator> (begin, end);
Not sure the “end” temporary really buys much, could just be:
return iterator_pair<const_reverse_iterator> (begin, region_begin);
> + }
>
> basic_block bb;
> gimple_stmt_iterator region_begin;
> gimple_stmt_iterator region_end;
> +
> + _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared
> *);
> + ~_bb_vec_info ();
> +
> } *bb_vec_info;
I realise it just ended up this way through the various iterations,
but I think the constructor and destructor should go between the
new types and the new member functions, rather than at the end.
The coding conventions say;
- first define all public types,
- then define all non-public types,
- then declare all public constructors,
- then declare the public destructor,
- then declare all public member functions,
- then declare all public member variables,
There should be no blank line before the final “}”.
OK with those changes, and thanks for your patience :-)
Richard
