Re: [PATCH] Improve portability across compilers

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.

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

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.
- why is the -Wno-error=unused-value being added there now too?

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

Amos
Received on Fri Aug 23 2013 - 07:21:10 MDT