Re: [PATCH] Improve portability across compilers

On 24/08/2013 5:27 a.m., Kinkie wrote:
> 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.

Clang complaining about those?

Two thing to keep in mind:
  * For automake 1.13 we will shortly have to be having to remove the
INCLUDES macro in the .am and spread its contents between LDFLAGS,
CFLAGS, CXXFLAGS, etc properly.

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

Okay. If those are older than the clang built into FreeBSD 9/10 by
default (IIRC they were) we don't really care, they also lack C++11
functionality we want to make use of.

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

I seem to remember separate sectiosn of switch case and if statements
early and late in the configure.ac the one you are editing here setting
up the details for --enable-strict-error-detection variables. And a
later one doing compiler-specific and OS-specific tuning for things like
-g / -stdc++11, march-native, etc etc. That later flags setup was still
quite messy and spread over a few different if statements with switches
inside. (it has been nagging at my OCD tendencies every time I see bits
of it).

Amos
Received on Sat Aug 24 2013 - 05:23:42 MDT