Re: [PATCH v2 5/7] name-hash: perf improvement for lazy_init_name_hash

From: Stefan Beller <sbeller@google.com>
Date: Mar 23, 2017, 17:45 UTC
Message-ID: <CAGZ79kZKri4jsnGCWhpV_WjSef5DxSwMTxYOour9kjzTWOFcxA@mail.gmail.com>
In-Reply-To: <e987714e-473f-cad1-159b-18459ffeb241@ramsayjones.plus.com>

On Thu, Mar 23, 2017 at 8:25 AM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 20 quoted lines

>> +static struct dir_entry *hash_dir_entry_with_parent_and_prefix(
>> +     struct index_state *istate,
>> +     struct dir_entry *parent,
>> +     struct strbuf *prefix)
>> +{
>> +     struct dir_entry *dir;
>> +     unsigned int hash;
>> +     int lock_nr;
>> +
>> +     /*
>> +      * Either we have a parent directory and path with slash(es)
>> +      * or the directory is an immediate child of the root directory.
>> +      */
>> +     assert((parent != NULL) ^ (strchr(prefix->buf, '/') == 0));
>
> sparse complains about 'Using plain integer as a NULL pointer', (the
> return from strchr() is NULL, if '/' is not found) so maybe:
>
> +       assert((parent != NULL) ^ (strchr(prefix->buf, '/') == NULL));
>

Also this seems part of the actual shipped code, not testing code. In that case we prefer

    if (<condition>)
        die("BUG: <description>");

This is because asserts may be omitted by the compiler, when compiled with NDEBUG.

However it is preferable to have these error checking code in the shipped product, as opposed to users running into that edge case and reporting an error which is totally non-obvious.

Thanks, Stefan