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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Aug 13, 2015 at 12:15 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> 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