Re: SqString

-- Please note that I am only [briefly] responding in hope to improve
future SqString patches. I do realize that this is no longer relevant to
Squid 3.0.

On Sat, 2007-05-26 at 18:41 +1200, Amos Jeffries wrote:
> Alex Rousskov wrote:
> >
> > And you want to remove those methods because ...? Can you just rename
> > them to match the STL naming scheme?
>
> See for yourself:
>
> int
> String::caseCmp (char const *aString) const
> {
> return strcasecmp(buf(), aString);
> }
>
> int
> String::caseCmp (char const *aString, size_t count) const
> {
> return strncasecmp(buf(), aString, count);
> }
>
> Now spot the benefit in that duplicated code...

I do not see code duplication and can name a couple of benefits. Please
see below for some of them.

> Secondly, they were a custom API to the old String:: that was used in
> only a few places with the non-wrapped version still used over most of
> the code.

True, but hardly an API's fault. If we want to stick with a custom
String long-term, the "most of the code" can be changed to use the right
API.

> And how is the above different from:
>
> strncasecmp(str.buf(), aStr, count);
>
> OR
>
> strncasecmp((char*)str, aStr, count);
> [ with (char*)str ==> str.buf() ].
>

There are a few differences: The calls with the old API are shorter and
less confusing (e.g., do I cast the first parameter to char*, use
str.buf(), or just str with the new API?). I can also track and modify
the original code in one place, without searching for and modifying all
strncasecmp calls.

> > Does the problem boil down to not
> > having an STL equivalent of some custom String methods?
>
> No as the above shows they are _excatly_ equivalent.

Obviously, we disagree.

> > 1) String::buf() did not return NULL for an empty but initialized
> > String. SqString::empty() does:
>
> WTF? here is the original code:
> char const *
> String::buf() const
> {
> return buf_;
> }
>
> WHERE: buf_ is either NULL, or a char*. NO initialisation at all going
> on there.
>
> WHERE empty() returns (size()==0 || buf_ == NULL);
> TRUE - IFF buf_ == char*
> FALSE - IFF buf_ ==

I am saying that !String::buf() might return "false" when
String::empty() returns true. When you substitute one for the other, I
begin to worry about new bugs. Consider an empty string (""), for
example. Does it have zero size? Is its buf() NULL?

What may look like trivial polishing may result in hard-to-find bugs. We
should not mix the two steps (zero-effect core API change and
polishing), if possible.

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

> >
> > 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];

> I am, so long as your who wrote that code are using the int type given
> to eOff properly (as I see you doing in that code) as a whole-number
> offset in bytes from the start of buf_.

I was talking about a standard C++ std::string here (the eventual
zero-code-change replacement for SqString, right?) not our custom String
code. I do not know how "address of a character" operation is defined
for STL strings and whether there are any performance implications to
worry about.

HTH,

Alex.
Received on Tue May 29 2007 - 14:35:57 MDT