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

[Kinkie]

On Mon, Aug 24, 2015 at 9:41 PM, Alex Rousskov <
rousskov at measurement-factory.com> wrote:

> On 08/24/2015 01:21 PM, Kinkie wrote:
>
> > On Mon, Aug 24, 2015 at 8:37 PM, Alex Rousskov wrote:
> >
> >     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.
> >
> >
> > I may have difficulty expressing myself here. The iterators work just
> > fine outside the range. Dereferencing them is another matter, as there
> > is no valid symbol to cast them to.
>
> Remove that paragraph then. It is a Bad Idea to try to document how
> iterators work in general when describing a specific class. Incrementing
> or dereferencing end() or equivalent is not going to work well, but we
> should not document that.
>

How about:
 * The iterator does not check for bounds when incrementing or decrementing,
 * that responsibility is left to the caller
?


> >     >> + *   zeroth, first, second, onePastLast, fourth
> >
> >
> >     Please make onePastLast last.
>
>
> > I'm trying to highlight the fact that
> > the dereferencing will only happen on EnumType::first and
> > EnumType::second. Any suggestion on how to clearly express it is welcome.
>
> 1. Replace the current enum values with five color names: white, blue,
> red, orange, pink.
>
> 2. Iterate from blue to orange.
>
> 3. Add a do_stuff(v) call comment saying that it will be called with red
> and blue values only.
>

Done


> For extra points, order colors by hue :-).
>

Dammit Jim, I'm an engineer, not a painter! :P


> >     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.
>
>
> > I'm trying to detail that in the documentation for EnumRangeT. Not
> > clearly enough, I guess.
>
>
> My bad. Sorry I missed that paragraph. It is OK. I would remove the
> "Otherwise you will have to ..." suggestion though -- no need to
> instruct folks to write bad code.
>

Ok.


> Note that one _can_ use EnumRange() to iterate whole enums; they just
> need a marker to do that. You do not have to polish your comment though.
>

That'd be quite redundant, it's stated well enough already that the last
element in the range is not going to be involved in the loop.
Thanks, and sorry for missing your comments earlier on. Updated patch will
follow after I address Amos' comments as well.

  Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150827/c6645167/attachment.html>