[squid-dev] [CODE] iterating over enums

[Alex Rousskov]

On 08/12/2015 09:36 AM, Kinkie wrote:

> So I went ahead and just implemented it, starting from your observations.
> It's a bit more complex that your proposal, as I took the chance to
> implement bidirectional forward and reverse iterators.
> The attached patch is in changeset 14235 of
> lp:~kinkie/squid/coverity-fixes, but since the code is quite well
> self-contained I'd like to submit it for audit as a standalone unit; If

Yes, submitting Enum iterators for review as a stand-alone patch is the
right thing to do. Hiding this and other significant changes under
"coverity fixes" cover is wrong IMO, even if those changes were prompted
by something Coverity has detected.

I would encourage you to show actual Squid usage examples (besides
artificial test cases), either in the patch itself or in the patch cover
email.

Also, please adjust the WholeEnum documentation. It is currently too
similar to EnumRange, including the identical first/summary line. It is
often best to document children classes in terms of parent classes
rather then repeating everything the parent documentation [currently]
says IMO. For example:

/// EnumRange for iterating all enum values, from enumBegin_ up to, but
// not including, enumEnd_. The two markers must be present in the enum.
template <typename Enum>
class WholeEnum ...


If you have not considered implementing WholeEnum without inheritance,
please do so. I speculate that lack of any data members may help inline
WholeEnum. Please note that I am just asking you to _consider_ this. I
do not have enough information to insist on it.


> +    typedef typename std::underlying_type<Enum>::type value_t;
> +    value_t current_value;

This type and data member is more about iteration position than "value"
(in STL sense). AFAICT, STL calls this type "iterator_type" and the data
member "current". I think that naming is better than yours.


> +    reverse_iterator rbegin() const { auto i = reverse_iterator(end_); ++i; return i; }
> +    reverse_iterator rend() const { auto i = reverse_iterator(begin_); ++i; return i; }

If these methods can be implemented simply as

  reverse_iterator rbegin() const { return ++reverse_iterator(end_); }
  reverse_iterator rend() const { return ++reverse_iterator(begin_); }

then please simplify.


I do not understand how rend() would actually work when iterating whole
enums! For a typical enum that starts with a zero-valued item, will the
rend() position (called current_value in your code) go negative?? Is
that actually guaranteed to work by the standard? If not, we may need to
require a begin-1 marker (which may be negative if needed)!

Are you sure you need reverse iterators at all?


[ EnumRange is not suitable for iterating up to the end of an enum that
does not have an end+1 marker. This limitation is not a bug, but may be
another reason to require all enums to have markers. ]


If possible, please add and use a global function to guess EnumRange
template parameter value from the actual function parameter types and
return the right EnumRange object, to avoid writing Enum type three
times when using EnumRangeT<>.

  template <typename Enum>
  EnumRangeT<Enum> EnumRange(Enum begin, Enum end) {
      return EnumRangeT<Enum>(begin, end);
  }


HTH,

Alex.



> On 08/05/2015 07:58 PM, Kinkie wrote:
> 
>> What I understand from the changes you propose is that:
> 
>> 1. instead of having explicit begin/end markers passed as template
>> arguments you propose to have a convention on begin/end markers.
> 
> There are several different things/layers mixed up here:
> 
> * If we want to use range loops for enums [in a sane way], then our
> enums should have begin/end markers. Many already do, but we will have
> to standardize their names and fill the gaps.
> 
> * The begin/end parameters should not be template parameters.
> 
> * If we need to iterate sub-ranges, then we should add EnumRangeT<Enum>
> class with begin/end as constructor parameters (see below).
> 
> 
>> 2. you skip the typedef fixing these parameters in favor of letting the
>> template parameters appear in the calls.
> 
> There are two separate issues/layers here:
> 
> * Naming an expression is a good idea, especially if that name is going
> to be used multiple times.
> 
> * Fewer template parameters are generally better if they can accomplish
> the same thing as more template parameters.
> 
> 
>> 1. looks interesting, especially as an enum missing those markers will
>> fail to compile. Maybe both approaches can be combined by using default
>> template arguments, what do you think?
>> template <typename C, C first = C::enumBegin_, C last_plus_one =
>> C::enumEnd_>
> 
> I think we should not specify iteration range boundaries as template
> parameters.
> 
> 
>> 3. we could maybe combine the benefits of beautiful range-for and ugly
>> explicit-for also for shorter ranges by having (in the class-based
>> variant), on top of what you have already proposed (untested)
> 
> Yes, if sub-range iteration using range loops is needed, then we should
> add EnumRangeT<Enum> class that does it. Sorry if I have not mentioned
> that explicitly.
> 
> Your sketch for that class is fine, but it will be a EnumRangeT class,
> separate from simpler WholeEnum, and we would try to define a function
> that will create that EnumRangeT object so that we only have to type
> "Numbers" zero or two times as illustrated below:
> 
> template <typename Enum>
> inline
> EnumRangeT<Enum> EnumRange(const Enum from, const Enum to) {
>     return EnumRangeT<Enum>(from, to);
> }
> 
> 
> So that instead of:
> 
>> for (auto i : WholeEnum<Numbers>(Numbers::one, Numbers::three)) {
>>  // do stuff
>> }
> 
> we could do:
> 
>   // iterating sub-range of a class enum:
>   for (auto i: EnumRange(Numbers::one, Numbers::three)) ...
> 
>   // iterating sub-range of a global C enum:
>   for (auto i: EnumRange(one, three)) ...
> 
>   // iterating a whole enum:
>   for (auto i: WholeEnum<Numbers>()) ...
> 
> 
> Again, the above range objects can be named (and the first two should be
> named if they are used more than once). For example:
> 
>   WholeEnum<Numbers> AllNumbers() { return WholeEnum<Numbers>(); }
> 
>   EnumRange<Numbers> FooBarNumbers() { return EnumRange(Numbers::foo,
> Numbers::bar); }
> 
> 
> 
>> It looks to me that I have crossed an itch you feel as well,
> 
> No, I am just trying to help you scratch your itch without scarring
> Squid code. I do not know whether your itch is real or imaginary.
> 
> 
> Cheers,
> 
> Alex.
>