On Tue, 2008-09-02 at 05:10 +0200, Kinkie wrote: > On Tue, Sep 2, 2008 at 12:21 AM, Alex Rousskov > <[EMAIL PROTECTED]> 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.
