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

[Kinkie]

On Mon, Aug 24, 2015 at 8:37 PM, Alex Rousskov <
rousskov at measurement-factory.com> 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.


> ++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()!
>

Yes, and it is.


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

I don't think that's needed. What I want to highlight in that comment is
that there's no preemptive checks on these out-of-bounds conditions
happening; this is I believe an analogy with what happens with many std::
iterators.


>
> >> + *   zeroth, first, second, onePastLast, fourth
>
>
> Please make onePastLast last.
>

That's actually intentional, even though the name may be unfortunate, and
could be renamed to "three". 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.


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


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

Yes, ok.


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

Ok.


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

Thank you, this is quite a lot already.


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