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.