Re: SqString

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.

>
>> Replacing it all with a single casting operator which explicitly forces
>> the magic casts to reverse from the expensive copy 'up' toward string
>> into a cheap cast 'down' to a c_str() call.
>
> I am not sure why we need that operator now. On the IRC, you said that
> it "it seems to be the only working alternative to complete removal of
> the change" but I do not understand why.

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

At this stage we actually have a three-way choice:
  - backout
  - add in c_str() every place its needed.
  - add forced down-casting.

since the down-caster is simply an inline of c_str() the last two
options are the same, one with more work, one with less. Same actions.

With the downcasting its only 'implicit' in the nature of we cannot say
'its used there, and there, and there' without a full code audit.
Being part of string we can be certain that it will be be used only
where the old methods/defines were valid.

>
> Moreover, if we must have implicit casts for the new API to work,
> perhaps we should back out the entire API change for now because we are
> not good at predicting the side-effects of such implicit casts.
>

It's one of the options.

>> Keep in mind this c_str() call being explicitly done all over squid at
>> present anyway. The only known risk is a buffer over- or under-run which
>> would occur anyway if the alternative explicit c_str() call was coded.
>
> Here is my problem: The original API change changed some strcmp-like
> calls. Some of the changes were incorrect. It is difficult for me to say
> whether removing strcmp overloading and adding implicit char* casts will
> expose all incorrect changes that we have not found yet. Unfortunately,
> I doubt it.
>
> 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.

Amos