[squid-dev] [CODE] iterating over enums

[Alex Rousskov]

On 08/13/2015 03:20 AM, Kinkie wrote:
> On Wed, Aug 12, 2015 at 9:11 PM, Alex Rousskov wrote:
>     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.


> ./src/HttpHdrCc.cc:HttpHdrCcType &operator++ (HttpHdrCcType &aHeader)
> ./src/HttpHdrSc.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type &aHeader)
> ./src/HttpHdrScTarget.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type
> &aHeader);
> ./src/HttpMsg.cc:HttpMsgParseState &operator++ (HttpMsgParseState &aState)
> ./src/errorpage.cc:err_type &operator++ (err_type &anErr)
> ./src/http/RequestMethod.cc:operator++ (Http::MethodType &aMethod)
> ./src/mem/old_api.cc:mem_type &operator++ (mem_type &aMem)
> 
> These are all used to iterate over enums from what I can tell.


By "actual Squid usage examples", I meant examples of your new code used
in Squid code (besides test cases) and not references to places where
the new code might be used in the future. Seeing real usage examples,
especially for unusual cases like iterating a sub-range is often very
helpful when reviewing a new API.

Similarly, proposing a replacement API without showing what changes it
would bring to Squid sources is setting a bad example/precedent, even if
these specific changes themselves are going to be welcomed by everybody.


>     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.
> 
> 
> As all classes are nonvirtual and everything is in the same translation
> unit, for now I would skip that. and avoid the code duplication.


I do not understand how "nonvirtual and everything is in the same
translation unit" is related to eliminating data members, but I do
appreciate you considering my request!


> 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.

As I said, I think these changes should be posted as a regular [PATCH]
and go through the normal review and commit process. "Coverity fixes"
commits should be limited to trivial, small changes. However, I
_personally_ may not need to review your [PATCH] if you post it, so you
may ask Amos whether he needs to review it before spending time on that
repost.


On an unrelated note, it just occurred to me that you can save a little
ink if you s/EnumIterator/Enumerator/g :-). Totally optional, of course.


Thank you,

Alex.