Re: SBuf review

On Fri, 2008-09-19 at 14:36 +1200, Amos Jeffries wrote:
 
> >> *B3* If a class does not have both dump() and print() methods, let's
> >> call the only printing method print(). This will make it easier to
> >> manipulate printable objects for debugging.
> >
> > The convention I followed is: dump() is for debugging, stats etc.
> > print() is for operator <<.
> > I can adhere to any other convention, as long as there's consensus -
> > ideally to the point where it's sanctioned in the coding guidelines.
>
> The whole stats output dump() system IIRC was partially completed moving
> to sstream << .
> So a specific sstream<< and ostream << 'friends' might be better than
> print() and dump() anyways.

Side note: Friends cannot be virtual and templated so, in general,
print() is a good idea, usually accompanied by a [templated] << operator
to "print" anything printable.

We probably need to agree on the definition/purpose of print(), dump(),
and other similar methods, but that is a separate question.

> >> *B2* If SBufStore is refcounted, why is it not using the RefCountable
> >> API we already have? If this is some kind of optimization, I would
> >> prefer to see it done after the code is reviewed, merged, and debugged.
> >
> > This code is very low-level. and it's also very self-contained. I aim
> > for maximum efficiency.
>
> Ditto on the RefCountable. This was the API I was asuming you would use
> anyway. It's already well debugged, and tested, and low-level inline.
> Just inherit from RefCount and use I think.
>
> If we find in testing that we need the efficiency, we can test the two.

RefCount efficiency is about the same as that of the proposed code. I
did not realize that when I made the original comment above. I forgot
that RefCountable stores a counter and was willing to trade a little
inefficiency for correct code. After looking at RefCountable, I can now
confidently mark the above issue as *B1*!

> >> *B1* SBufStore is missing a copy constructor and an assignment operator.
> >> You do not have to actually implement them, but you have to declare them
> >> to make sure the defaults are not used. Make them private if you think
> >> they should never be used.
>
> With a fatal assert or loud DBG_CRITICAL debugs() complaint to catch bad
> behavior.

I have seen folks declaring but not defining the forbidden methods. With
that approach, violations lead to link-stage errors, which is much
better than a runtime assert or debug. However, I do not know how
portable such trick is. Does anybody know?

On the other hand, I think we already use that in Squid so we can
continue doing so until caught (not defining something does not add
extra work).

> >> *B1* SBuf assignment does not handle "assignment to self".
> >
> > Right. Is it safe to implement that as a noop?
>
> Worst case, do it with a fatal assertion and we will find out in testing.

Please do not assert! Assignment to self is acceptable and unavoidable
when you do not know whether pointer/reference1 points/refers to the
same object as pointer/reference2:

        a = b; // this can be an assignment to self!

The best code supports assignment to self without a special if-statement
that checks for assignment to self. Such code is difficult to write
though and our standards are not that high. A simple (this == &them)
check would do in most cases, I think.

HTH,

Alex.
Received on Fri Sep 19 2008 - 04:05:27 MDT