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

[Rustad, Mark D]

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

> On Mon, Sep 22, 2014 at 09:50:54PM +0000, Rustad, Mark D wrote:
> * Fixing those is a good idea if the fixes are clean - I think we all
> agree by now that adding code just to shut up gcc is not nice.

Yes, but I think there are a few cases where it could be helpful. When
there is something exceptional that will throw a warning. In one of the
patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
the warning that is thrown for every entry of the syscall table when
compiled with clang. The code is right and doing exactly what is wanted,
so note the exception and make it shut up.

> * Then, even if all those warnings were fixed one fine day, the people
> who fix them would be fighting windmills because every new patch which
> adds new places causing those warnings would simply go in because the
> warnings are not visible in default builds.
> 
> So the question IMO turns into: are there some warnings which we should
> promote to default builds so that they get taken care of eventually...

I would generally be in favor of that over time, but I recognize that with
the range of architectures and toolchains that need to be supported, that
may be difficult.

>> Well, I have W=1 in my environment, so I don't even have to ask for it, I
>> just get it.
> 
> I think this was the initial use case we had in mind for W= - use it
> during development in order to have the compiler do extra checks to your
> code. And it has caught a couple of issues, FWIW.

Yes, not everyone building the kernel is a developer. I certainly get that.
Right now W=2 is kind of useless for developers because so much noise is
generated. With the number of people working on Linux, it may be enough if
a handful are taking a look at W=2 messages, but right now it is a pretty
awful task to even try to look.

>> Nested-externs, for example, can catch people gratuitously providing a
>> function prototype that could become a hazard, but some use of that may
>> be justified. The macros provide a way to specifically allow certain
>> instances while generally discouraging it. Of course if you never use
>> W=2 you may never catch those gratuitous declarations.
> 
> Sure, but the cost for fixing that is what bothers me. For that
> particular case, it probably would even be cleaner to add a
> nested-extern check to checkpatch instead of cluttering the code with
> those macros.

Generally there should be relatively few exceptions that need to be tagged.
If there were 1000, that would be way too many. The full series of my
patches had 90 instances. Some of the few that have been sent have been
resolved in better ways, which we all agree is better. Suppose that 40
can't be reasonably resolved in direct ways. Is that really costly? I'm
trying to understand what your perception of the cost is.

Perhaps checkpatch would be a better gatekeeper for new code. OTOH, some of
those nested externs have already been eliminated, so at least for now the
warning is serving a purpose since it is scrubbing existing code.

>> Hopefully the discussion is somewhat useful.
> 
> Well, it has become already, as you can see. :-D

I take it as a given that the kernel sometimes has special needs and needs
to do special things. The macros make it possible to mark those special
usages. I prefer adding the macros in a few places to eliminating a warning
altogether unless the warning is always useless. I'm sure we agree that we
don't want to ever turn on -pedantic! :-)

-- 
Mark Rustad, Networking Division, Intel Corporation

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