Wireshark · Wireshark-dev: Re: [Wireshark-dev] #ifdef mess

[Jaap Keuter <jaap.keuter@xxxxxxxxx>]

On 30-03-16 00:48, Joerg Mayer wrote:
> On Tue, Mar 29, 2016 at 03:34:38PM +0100, João Valverde wrote:
>> On 28-03-2016 23:30, Joerg Mayer wrote:
>>> I've been meaning to write this mail for some years now but finally got around to it.
>>>
>>> Earlier today I committed 30900b443b85a7e760d703ca3d6efe61df4fe623, which I'm
>>> incredibly unproud of because of readablity:
>>>
>>>  static void
>>> -get_reordercap_runtime_info(GString *str _U_)
>>> +get_reordercap_runtime_info(
>>> +#if defined(HAVE_LIBZ) && !defined(_WIN32)
>>> +    GString *str)
>>> +#else
>>> +   GString *str _U_)
>>> +#endif
>>>  {
>>>
>>> It fixes the error at hand, but that is about all the good I can say about it.
>>
>> It's only an error because -Werror=used-but-marked-unused was
>> enabled. Since the semantics of _U_ are *possibly* unused variable,
>> neither the warning nor the #ifdef should exist IMO.
> 
> I don't follow your logic here. Couldn't we by the same logic just remove the _U_
> and then blame the resulting warning turning erro on the unused warning? The idea
> or turning on many warnings is to find coding/logic bugs. Blaming the problem on
> turning on the warning is like shooting the messenger, i.e. plain wrong.

Hmm, not quit IMHO. When the programmer adds '_U_' to the argument (s)he's
expecting this variable not being used. That's an affirmative statement. It has
to be explicitly made. Leaving out '_U_' is a statement of the opposite, the
argument is expected to being used. This is the presumed default statement,
costing no effort.

Not using an argument that is stated as being used is indicative of a
programming error. With 'least amount of effort' paradigm in mind for the
interface, this is indeed unexpected.

Using an argument that is stated as not being used is indicative of nothing
really, other than that a use of this argument is being made in retrospect of
the initial assumption when the interface was designed.

> 
> IMO the root cause isn't this or that warning - it's that we have many #ifdef paths
> in our normal code. I won't have the time to really look into this until the end
> of April, but it looks like either nobody cares about this or I didn't manage to
> get the idea what really bugs me across, so I probably have to demonstrate the
> idea with a specific patch.
> 
> Thanks!
>   Jörg
> 

Does code quality (under the assumption that readability is helping quality)
suffer from many ifdef's? My firm believe is: Yes. We're poor code
(pre-)processors, barely capable of understanding and overseeing normal code,
let alone the obfuscation of ifdef's.

I therefore believe that although the warning has merit (there is a discrepancy
between the published function interface definition and function
implementation), it should not be indicative of an error, as the solution in
'real life' (somewhat more complex) code bases is more damaging to code
readability, and thereby quality, than that it solves.

Thanks,
Jaap