[squid-dev] [PATCH] mempools-nozero part 3: wordlist

[Kinkie]

Hi,
  resurrecting this thread.

On Mon, Aug 24, 2015 at 7:59 PM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 08/24/2015 09:30 AM, Kinkie wrote:
>
>> the one case in the whole codebase where a wordlist element was
>> legitimately deleted.
>
> This is not true. wordlistDestroy() deletes a wordlist object/element as
> well. You could (should?) have reimplemented wordlistDestroy() using the
> newly added wordlistChopHead() to reduce code duplication and friends
> declarations.

Good idea. Done in the attached patch.

>> The code looks odd, but this is an unfortunate consequence of
>> asymmetric expectations on the construct/destruct side: construct
>> side requrires C++ semantics for MEMPROXY_CLASS, destruct side
>> callers expect C semantics.
>
> The assymetry cause is a little different IMHO: While constructing code
> creates a single element at a time, wordlistDestroy() callers want to
> destroy the whole list at once.

Right.

>> +    // use wordlistDestroy instead
>> +    ~wordlist() = default;
>
> I would add a "does not delete data members" comment to emphasize that
> we know what is going on and are content with it. Not important though.

Fair enough. Changing.

>
>
>> +/** remove the first element in a wordlist, and return its key
>> + *
>> + * \note the returned key must be freed by the caller using safe_free
>> + * \note wl is altered so that it points to the second element
>> + * \return nullptr if pointed-to wordlist is nullptr.
>> + */
>> +char *wordlistChopHead(wordlist **);
>> +
>
> We need to document that the first element is destroyed. For example,
>
> /** Remove and destroy the first element while
>  *  preserving and returning its key.
> ...
>

LGTM, changing.


-- 
    Francesco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wordlist-review-alex-v1.patch
Type: text/x-diff
Size: 1355 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150925/89787c8c/attachment.patch>