Re: [PATCH] Improve portability across compilers

On Fri, Aug 23, 2013 at 9:21 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 23/08/2013 4:49 p.m., Alex Rousskov wrote:
>>
>> On 08/22/2013 11:25 AM, Kinkie wrote:
>>
>>> - clang is very strict on the number of parentheses in conditionals:
>>> as a way to double-check that the programmer knows what she's doing it
>>> will accept as valid
>>> if (foo==bar) {} and if ((foo=bar)) {} , and emit a warning (which
>>> then -Wall turns into an error) in case of if ((foo==bar)) {} and if
>>> (foo=bar) {}.
>>
>> Makes sense.
>>
>>
>>> This leads to bad results when the condition is a cpp macro, even
>>> worse when the macro is defined in an external library.
>>
>> Ouch. That is a clang bug IMHO.
>>
>> How about making COMPILE_STACK_EMPTY and friends into functions instead
>> of deleting them?
>>
>>> - if (!COMPILE_STACK_EMPTY)
>>> + if (!compile_stack.avail == 0)
>>
>> That would automatically avoid weird code like the one quoted above and
>> make code more self-documenting.
>>
>
> Better: if (!compile_stack.avail == false)
>
> for avoiding the integer magic conversion please.

Well, compile_stack.avail is actually an unsigned integer.
I've turned it into a clearer syntax, but that's it.

>>> - if (BN_is_zero(serial))
>>> + if BN_is_zero(serial) // BN_is_zero has its own set of surrounding
>>> parentheses
>>
>> Please no. Let's not violate the visual syntax of an if statement. If we
>> have to accommodate clang bugs, let's write
>>
>> if (BN_is_zero(serial) == true)
>>
>> or something like that.

Done.

>>
>
> Agreed.
>
> Also ...
>
> * in acinclude/compiler-flags.m4 I am worried that you are leaving
> -Qunused-arguments present in the variable labeled Werror.
> - IMO this needs to be a separate flag only added if a compiler flag check
> proves it necessary.

It is necessary, unless we completely overhaul our build system: the
"unused arguments" it refers to are unused _compiler_ arguments. Such
as for instance include directories added to the search path but which
contain no headers needed by the current translation unit.

> - why is the -Wno-error=unused-value being added there now too?

I'm removing it, it's apparently unneeded.
Although some clang versions _are_ complainign about unused return
values somewhere in the CBDATA_CLASS2 macro definition. I wasn't able
to track where exactly so far.

> We have a separate section for the compiler-specific default flags right? if
> not we need to add it.

I'm not sure about what you mean..

-- 
    /kinkie