Re: pseudo-specs for a String class

Thanks for this extra feedback. I appreciate the advice everyone is giving.
I'll have limited access to the net for the next couple of days so I
won't be able to answer to the points you make right away, I should
have the time to incorporate some of your suggestions by then.
 Are you!

On 9/2/08, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
> 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.
>
>
>

-- 
    /kinkie