SBuf review

On Thu, 2008-09-18 at 14:10 +0200, Kinkie wrote:

> The feature branch is at https://code.launchpad.net/~kinkie/squid/stringng

Here is another round of comments, based on the current SBuf.h in the
above repository. I have marked my comments with these prefixes:

*B1*: a blocking issue. I do not recommend proceeding much further until
this issue is resolved because the code may not be accepted or would
have to be changed significantly to address the issue.

*B2*: I am not sure yet, but this may become a blocker for me. If you
are not certain, please consider save us some time by implementing the
requested changes. If you are certain that I am wrong, a discussion
would be nice and may save us from trouble later.

*B3*: Everything else.

*B1* I still think that string-manipulation interfaces should be moved
into a String class.

*B2* I still think that having NULL Strings is a bad idea.

*B2* I am not sure placing your classes in include/ is better than
placing them in src/. We have discussed similar questions before. I do
not know whether there was consensus, but I hope we agreed that only
Squid-independent portability/wrapper code should live outside of src/.
I think your code will (or should) be Squid-dependent. It will make a
lot of things easier. It is difficult to develop outside of src/ and it
does not buy us anything in this case.

*B2* class outOfBoundsException: public std::exception

It is better to inherit all your customized exceptions from the existing
TextException. Otherwise, the catching code will become unnecessary long
and the exception class code will duplicate a lot of things.
TextException itself should be polished and split or renamed to
Exception, but that is a different topic. Most generic catching code
should catch TextExceptions (which includes their children).

*B1* Exception specifications: type method(...) throw(...);

Surprisingly (well, at least it was to me!), exception specifications
are a bad idea in virtually all cases except for empty specifications.
Empty specifications should also be avoided unless you Know What You Are
Doing and Absulutely Sure About That.

Please remove all your empty throw() exception specifications.

Please comment out (but do not remove) your non-empty exception
specifications. They are useful as documentation:
    type method(...); // throw(...)
or perhaps
    type method(...); // throws ...

Thinking I am nuts? Here is one of the authoritative references on this
fascinating subject: http://www.gotw.ca/publications/mill22.htm

*B1* outOfBoundsException::what() returns a string that does not exist
outside of that method. One more reason to move non-trivial code outside
of the header where we do not have to review it (yet) :-).

*B2* Move helper class declarations outside of SBuf. Class is not a
namespace. Nested classes complicate the overall impression from the API
and tempt you to do things you really should not.

*B1* dump() methods should be constant.

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

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

*B2* Please make SBufStore data members private. They are too sensitive
to expose them like that. With the sensitive info open, we will not
catch all the abuses (or spend too much time explaining why a public
interface should not be used).

*B3* Please rename SBufStore::init() to a more traditional allocate().
Even your own comment says that the block is not initialized!

*B2* init() is missing asserts that the buffer has not been allocated
twice. If that code is meant to be called immediately from constructors
only, then at least make it private and add a comment about that. It is
important to note that the object is in invalid state until init() has
been called.

*B1* SBufStore::init() and SBufStore in general should not change the
reference counter. Reference counting is the prerogative of smart
pointers, not the objects they point to (even if the object stores the
counter). You need to add a smart pointer so that the code becomes clear
and manageable. Again, I suggest using RefCountable API to start with.
It will be easy to replace/optimize later, with no significant effect on
user code.

Again, this is why I keep suggesting that a polished API is agreed on
first, before we start finding bugs in the unpolished API
implementation.

*B2* SBufStore constructors should be declared before init() and other
methods. Same for SBuf.

*B1* SBufStore(const SBuf &S) is not a copy constructor but the comment
says it is.

*B2* SBufStore(const SBuf &S) makes implicit conversions possible. Do we
really need these? Doesn't creating "store" from a buffer that uses that
"store" make you feel uncomfortable?

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

*B3* It is probably too early to review statistics, but "alloc" does not
seem to contain the "number of allocation operations". This is probably
because it is maintained in the code that does not allocate anything
(SBuf).

*B1* SBuf assignment does not handle "assignment to self".

*B1* Appending interface and/or comments do not make sense:

> 575 * Append to the supplied SBuf, <b>modifying</b> this.
> SBuf append(SBuf& S)

If we are appending TO the supplied SBuf, then the method should be
const (we would not be modifying this) and should be named appendTo. I
do not think we need such a method. If we are appending FROM the
supplied SBuf, then that SBuf should be const (we will not be modifying
it) and the comments should not talk about that supplied buffer
maintenance (again, because we do not modify it).

The return value should be documented, especially if you return a copy
of "this" buffer. Why would you do that?

Most of the above comments apply to other SBuf::append() methods as
well.

*B1* Please review all methods for const-correctness. Comparison
operators, for example, should have constant arguments.

*B2* I would remove numerous comments about what gets changed and what
does not. Const declarations already say as much!

*B3* Is dumpStats() needed? Is it replaceable with stats().dump() or
similar? "Smaller" APIs are often "better" and have fewer bugs.

*B3* slice() should probably be renamed to something like cut() or
chop() since you are not producing slices, you are cutting away
content.

*B2* slice() should be implemented by calling other, more primitive
chopping methods. Same for other methods that simply combine existing
simpler actions. Please do not force us to review the same code many
times.

*B2* I am not sure search methods should throw exceptions when the thing
is not found. Exceptions are expensive and complex things. Not finding
something is a common case. The searcher calling find() should assume
that the thing may not be there. If we want to provide a confident
searcher a convenient but safe method, lets have two methods:

    // returns invalid position if not found (like std::npos)
    Position find(...) const;
    // throws if not found
    Position findOrThrow(...) const;

This gives user a choice. A confident user will be able to use/pass
findOrThrow() results immediately, without checking, but safely.

*B1* Please move the Tokenizer class into its own header. FWIW, I may
not be able to review it until SBuf/String issues are resolved.

HTH,

Alex.
Received on Thu Sep 18 2008 - 17:26:41 MDT