Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 8:33 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:

> On Fri, Sep 19, 2014 at 08:29:33AM -0700, Jeff Kirsher wrote:
>> The following patches silence over 100,000 warnings in a W=2
>> kernel build. This series does most of it by using the compilers
>> diagnostic controls. The first patch in the series adds macros to
>> invoke the pragmas for those controls. Macros are provided for GCC
>> and clang. Although they are highly compatible in this area, macros
>> are provided for compiler-specific controls, and there is one
>> example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).
>> 
>> Some missing-field-initializers warnings were resolved using
>> the diagnostic control macros simply because so many lines
>> would have had to have been changed. At this stage Mark thought 
>> about avoiding possible merge issues. If the maintainer would 
>> rather resolve them by using designated initialization, just 
>> say so.
>> 
>> The combined effect of this patch series and his other patches
>> that did not use these diagnostic control macros was to reduce 
>> the number of W=2 warnings from 127,164 to 1,345!
> 
> Sorry but I don't see the point of actively adding macros to the code
> just so that gcc is happy. There's a reason why a bunch of warnings are
> disabled in the normal build and only enabled with the W= switch.
> 
> The W= things are supposed to be used when developing code and have the
> compiler tell you about *possible* issues. That doesn't mean though that
> we have to actively "fix" otherwise perfectly fine code.

The problem is that the kernel include files throw so many warnings that it really discourages anyone from ever going through them, even for a single driver. The warnings are far more valuable and usable when known acceptable usages are silenced.

> Having the need to actively go in and add code so that gcc doesn't issue
> obscure warnings is going too far, IMO.

Well, the whole series of patches that I made definitely went too far - only the first 5 out of about 30 have been posted, but if we can make some progress on generating fewer warnings out of the include files, I think it would be helpful. Already the patches that use them have triggered some activity that has resulted in resolving warnings without use of the macros, and I see that as much better than simply using the macros.

The macros can serve a useful purpose, but they should not be widely used. When to use them is definitely a judgement call. If the macros are accepted, it may be worth adding a checkpatch.pl warning for adding a DIAG_*IGNORE macro.

-- 
Mark Rustad, Networking Division, Intel Corporation

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail