Re: [PATCH v3] http: add support for specifying the SSL version

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 13, 2015, 16:37 UTC
Message-ID: <CAPig+cQj4-4tnZv1JkUZdGHzgL=x2f6Zg7JeYn5bBgp991WNhg@mail.gmail.com>
In-Reply-To: <CA+EOSBkOGzyOB-NRGTNm0b==OZH7eB=sZaGa0mRa4798_v-EHQ@mail.gmail.com>

On Thu, Aug 13, 2015 at 12:15 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:

Show 38 quoted lines

> 2015-08-13 18:11 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
>>> 2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>>>>> +       if (ssl_version != NULL && *ssl_version) {
>>>>> +               int i;
>>>>> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>>>> +                       if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>>>
>>>> This sort of loop is normally either handled by indexing up to a limit
>>>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>>>> (NULL, in this case). It is redundant to use both, as this code does.
>>> I do not think. sslversions[i].name can be null, see how the structure
>>> is initialized. No ?
>>
>> The initialization:
>>
>>     static struct {
>>        const char *name;
>>        long ssl_version;
>>        } sslversions[] = {
>>            { "sslv2", CURL_SSLVERSION_SSLv2 },
>>            ...
>>            { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>>            { NULL }
>>     };
>>
>> terminates the list with a NULL sentinel entry, which does indeed set
>> sslversions[i].name to NULL. When you know the item count ahead of
>> time (as you do in this case), this sort of end-of-list sentinel is
>> redundant, and complicates the code unnecessarily. For instance, the
>> 'sslversions[i].name != NULL' expression in the 'if':
>>
>>     if (sslversions[i].name != NULL && *sslversions[i].name ...
>>
>> is an unwanted complication. In fact, the '*sslversions[i].name'
>> expression is also unnecessary.
> I agree. But this is what  suggested me Junio: =). What do I have to do ?
> It becomes difficult to keep everyone happy: =)

You're referring to [1] in which Junio's example table initialization had the NULL sentinel. That approach is fine, and my earlier comment:

    This sort of loop is normally either handled by indexing up to a
    limit (ARRAY_SIZE, in this case) or by iterating until hitting a
    sentinel (NULL, in this case). It is redundant to use both...

wasn't saying that you shouldn't use the NULL sentinel. It said only that you should choose one approach rather than complicating the code unnecessarily by mixing the two.

So, your loop can either look like this, if you use the NULL sentinel:

    struct ssl_map *p = sslversions;
    while (p->name) {
        if (!strcmp(ssl_version, p->name))
            ...
    }

or like this, if you use ARRAY_SIZE:

    for (i = 0; i < ARRAY_SIZE(sslversions); i++) {
        if (!strcmp(ssl_version, sslversions[i].name))
            ...
    }

Each loop form is valid, and (other than the fact that the compiler knows the array size, thus slightly favoring the ARRAY_SIZE form) the choice of which of the above two forms to use isn't that important, and you can choose whichever you like, but please do choose one of the above two. If you feel that Junio would be happier with the NULL-sentinel form, then go with that.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275773