<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 12, 2015 at 9:11 PM, Alex Rousskov <span dir="ltr"><<a href="mailto:rousskov@measurement-factory.com" target="_blank">rousskov@measurement-factory.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 08/12/2015 09:36 AM, Kinkie wrote:<br>
<br>
> So I went ahead and just implemented it, starting from your observations.<br>
> It's a bit more complex that your proposal, as I took the chance to<br>
> implement bidirectional forward and reverse iterators.<br>
> The attached patch is in changeset 14235 of<br>
> lp:~kinkie/squid/coverity-fixes, but since the code is quite well<br>
> self-contained I'd like to submit it for audit as a standalone unit; If<br>
<br>
</span>Yes, submitting Enum iterators for review as a stand-alone patch is the<br>
right thing to do. Hiding this and other significant changes under<br>
"coverity fixes" cover is wrong IMO, even if those changes were prompted<br>
by something Coverity has detected.<br>
<br>
I would encourage you to show actual Squid usage examples (besides<br>
artificial test cases), either in the patch itself or in the patch cover<br>
email.<br></blockquote><div><br>./src/HttpHdrCc.cc:HttpHdrCcType &operator++ (HttpHdrCcType &aHeader)<br>./src/HttpHdrSc.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type &aHeader)<br>./src/HttpHdrScTarget.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type &aHeader);<br>./src/HttpMsg.cc:HttpMsgParseState &operator++ (HttpMsgParseState &aState)<br>./src/errorpage.cc:err_type &operator++ (err_type &anErr)<br>./src/http/RequestMethod.cc:operator++ (Http::MethodType &aMethod)<br>./src/mem/old_api.cc:mem_type &operator++ (mem_type &aMem)<br><br></div><div>These are all used to iterate over enums from what I can tell. There are for sure more cases using different techniques, that are less trivial to detect.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, please adjust the WholeEnum documentation. It is currently too<br>
similar to EnumRange, including the identical first/summary line. It is<br>
often best to document children classes in terms of parent classes<br>
rather then repeating everything the parent documentation [currently]<br>
says IMO. For example:<br>
<br>
/// EnumRange for iterating all enum values, from enumBegin_ up to, but<br>
// not including, enumEnd_. The two markers must be present in the enum.<br>
template <typename Enum><br>
class WholeEnum ...<br>
<br>
<br>
If you have not considered implementing WholeEnum without inheritance,<br>
please do so. I speculate that lack of any data members may help inline<br>
WholeEnum. Please note that I am just asking you to _consider_ this. I<br>
do not have enough information to insist on it.<br></blockquote><div><br></div><div>As all classes are nonvirtual and everything is in the same translation unit, for now I would skip that. and avoid the code duplication.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +    typedef typename std::underlying_type<Enum>::type value_t;<br>
> +    value_t current_value;<br>
<br>
This type and data member is more about iteration position than "value"<br>
(in STL sense). AFAICT, STL calls this type "iterator_type" and the data<br>
member "current". I think that naming is better than yours.<br></blockquote><div><br></div><div>Ok<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> +    reverse_iterator rbegin() const { auto i = reverse_iterator(end_); ++i; return i; }<br>
> +    reverse_iterator rend() const { auto i = reverse_iterator(begin_); ++i; return i; }<br>
<br>
If these methods can be implemented simply as<br>
<br>
  reverse_iterator rbegin() const { return ++reverse_iterator(end_); }<br>
  reverse_iterator rend() const { return ++reverse_iterator(begin_); }<br>
<br>
then please simplify.<br></blockquote><div><br></div><div>Ok.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I do not understand how rend() would actually work when iterating whole<br>
enums! For a typical enum that starts with a zero-valued item, will the<br>
rend() position (called current_value in your code) go negative?? Is<br>
that actually guaranteed to work by the standard? If not, we may need to<br>
require a begin-1 marker (which may be negative if needed)!<br></blockquote><div><br></div><div>It'll go negative, or in case of unsigned underlyng_type, wrap. It'll still work (added a test for that), to be sure.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Are you sure you need reverse iterators at all?<br></blockquote><div><br></div><div>No, not sure; I added them mostly for completeness sake.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[ EnumRange is not suitable for iterating up to the end of an enum that<br>
does not have an end+1 marker. This limitation is not a bug, but may be<br>
another reason to require all enums to have markers. ]<br></blockquote><div><br></div><div>I've added documentation to highlight that corner case so it doesn't come unexpected.<br></div><div>I've considered changing it so that it includes the last value in the range, but in the end decided against it as it's inconsistent with common behavior.<br></div><div>In some cases it may not be feasible to have markers. Maybe the enum comes from a library, or maybe markers may hurt some code path for all we know. As long as the behavior is well documented and consistent, I don't think it'll be a problem.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If possible, please add and use a global function to guess EnumRange<br>
template parameter value from the actual function parameter types and<br>
return the right EnumRange object, to avoid writing Enum type three<br>
times when using EnumRangeT<>.<br>
<br>
  template <typename Enum><br>
  EnumRangeT<Enum> EnumRange(Enum begin, Enum end) {<br>
      return EnumRangeT<Enum>(begin, end);<br>
  }<br>
<br></blockquote><div><br></div><div>Done.</div><div>I'm attaching the resulting files. If that's OK by you, I'll start using them and merge together with the next round of coverity-fixes.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
HTH,<br>
<br>
Alex.<br>
<div><div><br>
<br>
<br>
> On 08/05/2015 07:58 PM, Kinkie wrote:<br>
><br>
>> What I understand from the changes you propose is that:<br>
><br>
>> 1. instead of having explicit begin/end markers passed as template<br>
>> arguments you propose to have a convention on begin/end markers.<br>
><br>
> There are several different things/layers mixed up here:<br>
><br>
> * If we want to use range loops for enums [in a sane way], then our<br>
> enums should have begin/end markers. Many already do, but we will have<br>
> to standardize their names and fill the gaps.<br>
><br>
> * The begin/end parameters should not be template parameters.<br>
><br>
> * If we need to iterate sub-ranges, then we should add EnumRangeT<Enum><br>
> class with begin/end as constructor parameters (see below).<br>
><br>
><br>
>> 2. you skip the typedef fixing these parameters in favor of letting the<br>
>> template parameters appear in the calls.<br>
><br>
> There are two separate issues/layers here:<br>
><br>
> * Naming an expression is a good idea, especially if that name is going<br>
> to be used multiple times.<br>
><br>
> * Fewer template parameters are generally better if they can accomplish<br>
> the same thing as more template parameters.<br>
><br>
><br>
>> 1. looks interesting, especially as an enum missing those markers will<br>
>> fail to compile. Maybe both approaches can be combined by using default<br>
>> template arguments, what do you think?<br>
>> template <typename C, C first = C::enumBegin_, C last_plus_one =<br>
>> C::enumEnd_><br>
><br>
> I think we should not specify iteration range boundaries as template<br>
> parameters.<br>
><br>
><br>
>> 3. we could maybe combine the benefits of beautiful range-for and ugly<br>
>> explicit-for also for shorter ranges by having (in the class-based<br>
>> variant), on top of what you have already proposed (untested)<br>
><br>
> Yes, if sub-range iteration using range loops is needed, then we should<br>
> add EnumRangeT<Enum> class that does it. Sorry if I have not mentioned<br>
> that explicitly.<br>
><br>
> Your sketch for that class is fine, but it will be a EnumRangeT class,<br>
> separate from simpler WholeEnum, and we would try to define a function<br>
> that will create that EnumRangeT object so that we only have to type<br>
> "Numbers" zero or two times as illustrated below:<br>
><br>
> template <typename Enum><br>
> inline<br>
> EnumRangeT<Enum> EnumRange(const Enum from, const Enum to) {<br>
>     return EnumRangeT<Enum>(from, to);<br>
> }<br>
><br>
><br>
> So that instead of:<br>
><br>
>> for (auto i : WholeEnum<Numbers>(Numbers::one, Numbers::three)) {<br>
>>  // do stuff<br>
>> }<br>
><br>
> we could do:<br>
><br>
>   // iterating sub-range of a class enum:<br>
>   for (auto i: EnumRange(Numbers::one, Numbers::three)) ...<br>
><br>
>   // iterating sub-range of a global C enum:<br>
>   for (auto i: EnumRange(one, three)) ...<br>
><br>
>   // iterating a whole enum:<br>
>   for (auto i: WholeEnum<Numbers>()) ...<br>
><br>
><br>
> Again, the above range objects can be named (and the first two should be<br>
> named if they are used more than once). For example:<br>
><br>
>   WholeEnum<Numbers> AllNumbers() { return WholeEnum<Numbers>(); }<br>
><br>
>   EnumRange<Numbers> FooBarNumbers() { return EnumRange(Numbers::foo,<br>
> Numbers::bar); }<br>
><br>
><br>
><br>
>> It looks to me that I have crossed an itch you feel as well,<br>
><br>
> No, I am just trying to help you scratch your itch without scarring<br>
> Squid code. I do not know whether your itch is real or imaginary.<br>
><br>
><br>
> Cheers,<br>
><br>
> Alex.<br>
><br>
<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div>    Francesco</div>
</div></div>