All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Rientjes <rientjes@google.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Christoph Lameter <cl@linux.com>,
	Pekka Enberg <penberg@kernel.org>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	linux-mm@kvack.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Mark Brown <broonie@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	David Miller <davem@davemloft.net>
Subject: Re: [patch] compiler, clang: suppress warning for unused static inline functions
Date: Wed, 31 May 2017 14:45:31 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1705311436490.82977@chino.kir.corp.google.com> (raw)
In-Reply-To: <CAD=FV=Xi7NjDjsdwGP=GGS9p=uUpqZa7S=irNOFmhfD1F3kWZQ@mail.gmail.com>

On Wed, 31 May 2017, Doug Anderson wrote:

> > Again, I defer to maintainers like Andrew and Ingo who have to deal with
> > an enormous amount of patches on how they would like to handle it; I don't
> > think myself or anybody else who doesn't deal with a large number of
> > patches should be mandating how it's handled.
> >
> > For reference, the patchset that this patch originated from added 8 lines
> > and removed 1, so I disagree that this cleans anything up; in reality, it
> > obfuscates the code and makes the #ifdef nesting more complex.
> 
> As Matthias said, let's not argue about ifdeffs and instead talk about
> adding "maybe unused".  100% of these cases _can_ be solved by adding
> "maybe unused".  Then, if a maintainer thinks that an ifdef is cleaner
> / better in a particular case, we can use an ifdef in that case.
> 
> Do you believe that adding "maybe unused" tags significantly uglifies
> the code?  I personally find them documenting.
> 

But then you've eliminated the possibility of finding dead code again, 
which is the only point to the warning :)  So now we have patches going to 
swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle 
__maybe_unused throughout the code to not increase LOC in select areas but 
then we can't find dead code again.

My suggestion is to match gcc behavior and if anybody is in the business 
of cleaning up truly dead code, send patches.  Tools exist to do this 
outside of relying on a minority compiler during compilation.  Otherwise, 
this is simply adding more burden to already swamped maintainers to 
annotate every single static inline function that clang complains about.  
I'd prefer to let them decide and this will be the extent of my 
participation in this thread.

WARNING: multiple messages have this Message-ID (diff)
From: David Rientjes <rientjes@google.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Christoph Lameter <cl@linux.com>,
	Pekka Enberg <penberg@kernel.org>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	linux-mm@kvack.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Mark Brown <broonie@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	David Miller <davem@davemloft.net>
Subject: Re: [patch] compiler, clang: suppress warning for unused static inline functions
Date: Wed, 31 May 2017 14:45:31 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1705311436490.82977@chino.kir.corp.google.com> (raw)
In-Reply-To: <CAD=FV=Xi7NjDjsdwGP=GGS9p=uUpqZa7S=irNOFmhfD1F3kWZQ@mail.gmail.com>

On Wed, 31 May 2017, Doug Anderson wrote:

> > Again, I defer to maintainers like Andrew and Ingo who have to deal with
> > an enormous amount of patches on how they would like to handle it; I don't
> > think myself or anybody else who doesn't deal with a large number of
> > patches should be mandating how it's handled.
> >
> > For reference, the patchset that this patch originated from added 8 lines
> > and removed 1, so I disagree that this cleans anything up; in reality, it
> > obfuscates the code and makes the #ifdef nesting more complex.
> 
> As Matthias said, let's not argue about ifdeffs and instead talk about
> adding "maybe unused".  100% of these cases _can_ be solved by adding
> "maybe unused".  Then, if a maintainer thinks that an ifdef is cleaner
> / better in a particular case, we can use an ifdef in that case.
> 
> Do you believe that adding "maybe unused" tags significantly uglifies
> the code?  I personally find them documenting.
> 

But then you've eliminated the possibility of finding dead code again, 
which is the only point to the warning :)  So now we have patches going to 
swamped maintainers to add #ifdef's, more LOC, and now patches to sprinkle 
__maybe_unused throughout the code to not increase LOC in select areas but 
then we can't find dead code again.

My suggestion is to match gcc behavior and if anybody is in the business 
of cleaning up truly dead code, send patches.  Tools exist to do this 
outside of relying on a minority compiler during compilation.  Otherwise, 
this is simply adding more burden to already swamped maintainers to 
annotate every single static inline function that clang complains about.  
I'd prefer to let them decide and this will be the extent of my 
participation in this thread.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  parent reply	other threads:[~2017-05-31 21:45 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-24 21:01 [patch] compiler, clang: suppress warning for unused static inline functions David Rientjes
2017-05-24 21:01 ` David Rientjes
2017-05-24 21:22 ` Matthias Kaehlcke
2017-05-24 21:22   ` Matthias Kaehlcke
2017-05-24 21:32   ` Andrew Morton
2017-05-24 21:32     ` Andrew Morton
2017-05-24 23:28     ` Doug Anderson
2017-05-24 23:28       ` Doug Anderson
2017-05-31  0:10       ` David Rientjes
2017-05-31  0:10         ` David Rientjes
2017-05-31  1:53         ` Matthias Kaehlcke
2017-05-31  1:53           ` Matthias Kaehlcke
2017-05-31 15:53         ` Doug Anderson
2017-05-31 15:53           ` Doug Anderson
2017-05-31 18:26           ` Mark Brown
2017-05-31 21:45           ` David Rientjes [this message]
2017-05-31 21:45             ` David Rientjes
2017-05-31 22:31             ` Doug Anderson
2017-05-31 22:31               ` Doug Anderson
2017-06-01  0:01               ` Matthias Kaehlcke
2017-06-01  0:01                 ` Matthias Kaehlcke
2017-05-25  5:52   ` Ingo Molnar
2017-05-25  5:52     ` Ingo Molnar
2017-05-25 16:14     ` Matthias Kaehlcke
2017-05-25 16:14       ` Matthias Kaehlcke
2017-05-25 16:48       ` Joe Perches
2017-05-25 16:48         ` Joe Perches
2017-05-25 17:49         ` Matthias Kaehlcke
2017-05-25 17:49           ` Matthias Kaehlcke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.10.1705311436490.82977@chino.kir.corp.google.com \
    --to=rientjes@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=broonie@kernel.org \
    --cc=cl@linux.com \
    --cc=davem@davemloft.net \
    --cc=dianders@chromium.org \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@kernel.org \
    --cc=mka@chromium.org \
    --cc=penberg@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.