Re: SqString

On Sat, 2007-05-26 at 04:08 +1200, Amos Jeffries wrote:
> Alex Rousskov wrote:
> > On Fri, 2007-05-25 at 20:39 +1200, Amos Jeffries wrote:
> >
> >> I have just been experimenting with a few options short of a full backout.
> >>
> >> My initial idea of dropping the constructor drags the changes into areas
> >> the initial patch didn't touch. No go there.
> >
> > Yes, of course.
> >
> >> I have had some success dropping the str* overloading entirely.
> >
> > Since the old code did not have those, I suspect the new code should
> > have "complete success" without them.
>
> Ah, but it did. it just used a mix of some #define (2.x,3.0) and class
> methods (3.0) and added upper case letters in the name.

Class methods (versus overloaded globals) make a big difference when it
comes to silent type conversion (or lack of it). That is, in part, the
reason why the replacements in the new API where not functionally
equivalent to the original code.

> Ok let me make this clearer. the #defines and methods above did
> _explicit_ calls to c_str().

And you want to remove those methods because ...? Can you just rename
them to match the STL naming scheme? Does the problem boil down to not
having an STL equivalent of some custom String methods?

> > What I would like to see is a diff against the pre-new-API code showing
> > that no calls got rewritten. Renaming a method is fine, but most other
> > changes should be postponed. The new patch you posted does not show me
> > how the main code is affected by the two changes combined, which is the
> > only thing that matters. Comparing with the current, broken code is not
> > very informative...
>
> After you mentioned this on IRC I went back to my copy of the original
> patch.
> Attached is a pseudo-patch (has stuff elided so I don't expect it will
> map cleanly) showing all the str* changes and re-arranging you are
> worried about. All I have elided is the effected debug calls and
> straight name mods; buf() to c_str() etc.
> I have also left in the hash function-ptrs where I explicitly set the
> namespace on strcmp so as not to clash their old values with anything new.

I am uncomfortable with the changes in the latest patch because they
change the working code. I did not do full analysis, but here are a few
specific places that worry me:

1) String::buf() did not return NULL for an empty but initialized
String. SqString::empty() does:

> - if (target && sct->target.buf() && !strcmp (target,
> + if (target && !sct->target.empty() && !strcmp(target,

and

> - else if (!target && !sct->target.buf())
> + else if (!target && sct->target.empty())

and possibly elsewhere.

2) String::cmp does not just call strncmp so I cannot be sure this and
other similar changes are simple renaming:

> - if (buf->contentSize() >= protoPrefix.size() &&
> protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
> + if (buf->contentSize() >= protoPrefix.size() &&
> strncmp(protoPrefix, buf->content(), protoPrefix.size()) != 0) {

3) I am not sure C++ guarantees the following to be what we want it to
be so this might cause problems in the future:

> - strcmp(urlPath.buf() + eOff, e->key) == 0) {
> + strcmp(&urlPath[eOff], e->key) == 0) {

or

> - const char *s = uri.buf() + 7;
> + const char *s = &uri[7];

Are all of the above and other changes correct? I do not know. It is not
obvious to me that they are. If you are confident, and others do not
object, commit them (no need to prove the correctness to me!). If you
are not confident, let's back the whole thing out for now and then fix a
couple of problems that you have found while working on this. Once 3.0
is branched, let's discuss whether and how to switch to STL strings.

Thank you,

Alex.
Received on Fri May 25 2007 - 13:35:53 MDT