Re: pseudo-specs for a String class

On Tue, 2008-09-02 at 05:10 +0200, Kinkie wrote:
> On Tue, Sep 2, 2008 at 12:21 AM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
> > On Sun, 2008-08-31 at 14:07 +0200, Kinkie wrote:
> >
> >> I've gotten a bit forward, now I'm a bit at a loss about where to go next.
> >
> > I would move string, stream, and other formatting functions away from
> > this class.
> >
> > I would move searching and comparison functions away from this class.
> >
> > I would probably remove default conversion from char*. It buys you very
> > little in Squid low-level Buffer context, but can introduce serious
> > problems (we have seen some of that during the previous BetterString
> > attempts).
>
> I'm trying to make the class as useable as possible.
> Subclassing to add those operations is of course a possibility, I
> invite you to check the code out (it has a quite rich test/example
> section).

Test/examples do not matter in this particular case. What matters is the
unexpected and expensive implicit conversions that are pretty much
guaranteed to appear in the real code if implicit conversion from char*
is allowed. We cannot avoid all implicit conversions, but I would still
recommend removing the implicit conversion from char* from any
buffering- or string-related class.

> >> char *exportCopy(void);
> >> char *exportRefIKnowWhatImDoing(void);
> >
> > What do these return and how do they differ?
>
> The first exports a copy of the KBuf contents (potentially expensive
> but clear from the point of view of memory management), the latter
> exports a reference to the internal storage. Very cheap but quite
> dangerous as the underlying storage may be moved away. Thus the name
> expresses a contract by which the caller declaares to know what she's
> doing.

Understood. Please document:

 - the mechanism for destroying/freeing a "safe" copy,
 - whether the "safe" copy is null-terminated,
 - the scope where it is safe to use the IKnowWhatImDoing reference.

> >> KBuf& operator = (char const *S, u_int32_t Ssize);
> >
> > Does C++ have a tertiary assignment operator?
>
> of course not; please conosider that interface is a mock-up. Actual
> functions are:
>
> assign(KBuf &)
> operator = (KBuf &)
> assign (const char*, size_t)
> assign (const char*)
> operator = (const char *)

OK. The conversion from char* string and lack of "const" comments apply
to all of these but the third one then.

> >> const int operator [] (int pos);
> >
> > Useless without size() and probably does not belong to a low-level buffer.
>
> size() has been implemented since.
> That operator returns -1 (EOF) on out-of-bounds. I agree it's probably
> useless, I'm know of throwing all possible ideas in, removing calls is
> always possible.

I agree that removing something is always possible. On the other hand,
reviewing "all possible ideas" APIs is painful and not very productive.
Also, it is usually easier to add than to remove.

> > There are a few places with methods arguments are references that are
> > missing "const". Please review.
>
> Ok. Are you referring to the mockup or did you check the actual code?

I am referring to the "Current interface" blob you posted, asking about
the direction to go next. If the actual interface is not the "current
interface", please post whatever we should be looking at.

Four more comments on the "current interface" blob. Sorry if these are
irrelevant to the actual work.

* Please document the results of invalid operations such as appending
more data than would fit or truncating more than there is. I would
recommend throwing an exception as the starting point, but whatever the
final/agreed solution is it needs to be documented and discussed.

* bool isNull();

Unlike for a pointer, being "null" is a strange state for a buffer.
Please consider renaming that method to isEmpty (if that is what you
meant). BTW, MemBuf::isNull is not isEmpty!

* KBuf nextToken(const char *delim); //strtok()

The above API would require removing the token from the object or
keeping the token pointer as additional data member. Neither approach
allows multiple concurrent string iterations. Is that intentional?
Should data iteration be external to the object holding the data?

* Many methods that probably do not modify the object are missing
"const" at the end.

> As already discussed, I have no objections to splitting the class'
> interface in low-level and high-level.
> I'm currently taking the pragmatic approach that the high-level
> functions going to be almost always used anyways, so might as well
> clump them together.

Not sure why clumping high- and low-level interfaces together is
pragmatic, but when do you expect to make the decision on whether the
final interface you recommend would be "clumped" or not? This decision
would affect a few key aspects of String/buffer classes, such as
construction and extraction, so it would be nice not to delay it for too
long.

Thank you,

Alex.
Received on Tue Sep 02 2008 - 04:31:04 MDT