Re: StringNg review request: SBuf.h

On Tue, 21 Sep 2010 19:25:37 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/21/2010 11:29 AM, Kinkie wrote:
>>> Review is based on lp:~kinkie/squid/stringng r9471.
>>
<snip>
>
>>>> /// Flag used by search methods to define case sensitivity
>>>> static const bool caseInsensitive = false;
>>>> static const bool caseSensitive = true;
>>>
>>> Convert to enum?
>>
>> What is the benefit? (my ignorance)
>
> Groups common names together, allows missing switch() value checks, may
> allow for safer/aggressive compiler optimization, does not force you to
> make up constant values, assures unique values by default, disables bad
> behavior such as taking caseSensitive address, etc. Not a big deal in
> this context though.
>

... can be defined in .h symbols without requiring special .cc global.

We also have src/mk-string-arrays.awk to turn enumName.h files into
enumName.cc containing const char* enumName_Str[] arrays for debugging. see
src/Makefile.am for usage

>
>>> It would be better to avoid bare "this" in the above comments. It is
>>> awkward
>>> to read.
>>
>> I can't think of any better way to call it. I've removed the
>> reference, but I'm not sure it's clearer.
>> Copy-paste:
>> /** compare to other SBuf, strcmp-style
>> *
>> * Compare two SBufs. behaves like strcmp(3) or strncmp(3)
depending
>> * on the value of the case_sensitive parameter
>
> Avoid repetition. You already document what the parameter does, below.
> And I think you meant strcasecmp(3). Consider:
>
> /** compare to another SBuf, strcmp-style

NOTE: If you have two distinct methods/objects needing identical long
descriptions doxygen offers the \see directive for de-duping local
descriptions.

>>>> /** Copy SBuf contents into user-supplied C buffer.
>>>> *
>>>> * Export a copy of the SBuf's contents into the user-supplied
>>>> * buffer, up to the user-supplied-length.
>>>> * The exported buffer is guarranteed to have a terminating \0,
>>>> but
>>>> * may truncate contents if the SBuf is too big.
>>>> * \return 0 if all is OK
>>>> * \return num the number of actually-copied chars, INCLUDING
the
>>>> \0
>>>> * \note uses xstrncpy as low-level function
>>>> */
>>>> size_type copy(char *buf, size_type buflen) const;
>>>
>>> Do we really need to zero-terminate the above? This low-level method
>>> may be
>>> handy for I/O which does not require zero-termination. We should
>>> minimize
>>> the number of APIs that terminate, IMO.
>>
>> Zero-termination is performed by xstrncpy(), AFTER the copy is done.
>> It doesn't cow().
>> Should I mention this in the documentation instead?
>
> My point is quite different. Do we really need to zero-terminate the
> copied data? I suspect that we do not and should not.
>
> The fact that your implementation currently does terminate because it
> uses xstrncpy() is irrelevant.
>
> Moreover, your implementation is buggy because xstrncpy does not
> guarantee a full copy for binary data. Our xstrncpy also writes beyond
> buflen and the standard strncpy does not guarantee termination. Get us
> out of this snake pit!
>
> The correct implementation should probably use memcpy().

Absolutely. memcpy of min(buflen, SBuf::size()).

Termination (if actually needed) in such a copy would be at the
explicitly-terminated point or potentially earlier for binary data
containing \0.

>
> I am guessing we do not care about overlapping regions in this specific
> method because copying from SBufs to SBuf is only possible if the two
> have different MemBlobs. The implementation should document this
> assumption or even assert it.

side tracked? above says you are discussing "Copy SBuf contents into
user-supplied C buffer".

>>>> int SBuf::plength() const
>>>
>>> Do we need to place the return type on its own line in .cci?
>>
>> Should get fixed by running the formatter tool.
>
> AFAIK, the formatter tool does not fix this, unfortunately. I see
> Polygraph and other copied code in Squid that is not formatted correctly

> because of this.

Yes. The formatter does not fix this. It's one of my remaining annoyances.
That and the inconsistent spacings between function name and (.

Kinkie:
 as for your earlier question scripts/source-maintenance.sh does all the
automated enforcement.
 The formater.pl does not adequately check astyle version so can lead to
big trouble if you don't have 1.22 installed. If in doubt run it on a
master.squid-cache.org checkout.

>>>> void SBuf::clear()
>>>> {
>>>> #if 0
>>>> //enabling this code path, the store will be freed and
>>>> reinitialized
>>>> store_=getStorePrototype(); //uncomment to actually free storage
>>>> upon
>>>> clear()
>>>> #else
>>>> //enabling this code path, we try to release the store without
>>>> deallocating it.
>>>> // will be lazily reallocated if needed.
>>>> if (store_->RefCountCount() == 1)
>>>> store_->bufUsed=0;
>>>
>>> Please remove this bufUsed optimization. It appears to be broken (you
>>> increment bufUsed inconsistently), it is complex, and we can always
add
>>> it
>>> later.
>>
>> Done. I've switched the #if 0 around
>
> Which was the wrong change, IMO (see above). I just asked for the
> bufUsed-related lines to be removed, and not the correct algorithm to be

> replaced with the wrong one.
>
> In general, please avoid unrelated changes to the code under review
> until the code is accepted. Otherwise, you create a moving target for
> the reviewers.
>
>
> In fact, I like the bufUsed optimization a lot now, after I realized
> what your grow() and canAppend() hacks actually do! We should restore
> it as it may be very important in common cases. However, we should not
> be accessing refCount or bufUsed directly like that (here and elsewhere
> in SBuf). MemBlob should have an API to keep used portion size
> up-to-date. We need to come back to this after the other bugs are fixed.

> Please remind me if I forget or add a TODO.

You have not said it Alex but I shall.
Remove the #if case entirely kinkie. Getting things going right with the
#else case is far more important. Alex review covers whats needed to fix
that case.

Amos
Received on Wed Sep 22 2010 - 04:00:46 MDT