[squid-dev] [PATCH] Coverity-fixes, part 1: EnumIterator

[Alex Rousskov]

On 08/24/2015 12:08 PM, Amos Jeffries wrote:
> On 25/08/2015 4:49 a.m., Kinkie wrote:

> + * behavior is undefined if the iterator
> + * is incremented (or decremented) outside the range representing valid
> + * enum symbols

I hope that behavior is well defined: For many enums/iterators,
iterators are "incremented outside the range representing valid enum
symbols" to reach end() and rend(). I asked you about these beyond-range
increments, and you assured me that they work fine. We should not add a
comment saying otherwise.

++end() may be undefined, but the result of ++i should be well defined
for an i value pointing to the last (or first) enum value. That result
is what we usually compare with end() or rend()!

If you changed your mind, we have to remove reverse iterators and
require end markers for all enums (as one of the possible solutions).
That would guarantee that iterators are not incremented or decremented
beyond the enum symbols range.



>> + *   zeroth, first, second, onePastLast, fourth


Please make onePastLast last.


You might want to add a comment that EnumRange() cannot be used to
iterate whole enums without [end] markers and, hence, all enums that
require iteration of the entire range must have markers.


> * the iterator_type being used in static_cast before the typedef _looks_
> very dangerous (even if its fine and works).
>  - I would place typedef up the top in their own protected area to avoid
> a) depending on implicit compiler behaviour and b) future developer
> mistakes/misunderstanding.

And you can make the typedef public to simplify.


> I dont see the first-user code that we normally require for accepting
> these. I know its coming with a rather big followup, just would be good
> if there was a simple use that could be used as exemplar.

I have asked for usage examples in regular Squid code as well. I agree
that they should become a part of this patch, but I still do not insist
on that.


I do not have any other new comments and have no objections to the patch.


Alex.