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

[Alex Rousskov]

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.


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


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


> +/** 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.
...


I did not do a full audit and have no objections.


Thank you,

Alex.