[cfe-commits] Fwd: Clang change to detect and warn about unused private members

[Daniel Jasper]

Forgot to CC the list..

---------- Forwarded message ----------
From: Daniel Jasper <djasper at google.com>
Date: Thu, Jan 12, 2012 at 7:26 PM
Subject: Re: [cfe-commits] Clang change to detect and warn about unused
private members
To: Nico Weber <thakis at chromium.org>


Hi Nico,

comments inline.

On Wed, Jan 11, 2012 at 10:49 PM, Nico Weber <thakis at chromium.org> wrote:

> Hi Daniel,
>
> On Wed, Jan 11, 2012 at 1:11 AM, Daniel Jasper <djasper at google.com> wrote:
> > Hi,
> >
> > the attached change is designed to detect and warn about private unused
> > members of C++ classes. It checks whether a class is fully defined in the
> > current translation unit, i.e. all methods are either defined or pure
> > virtual and all friends are defined. Otherwise, the private member could
> be
> > used by a function defined in another translation unit.
>
> that's a cool warning! Here are a few cases where it flags false
> positives (it finds tons of true positives too, but those are boring
> :-) ).
>
> It flags |stackArray| in ICU's cmemory.h. This is declared for storage
> and accessed through pointer aliasing, probably not much that can be
> done about this:
>
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/icu/source/common/cmemory.h&exact_package=chromium&q=file:cmemory.h&l=285


As you said in the other email, it is the wrong code line and I think, it
is correct to warn about the other |stackArray|.


>
> In flags StaticResource::instance_ in v8's utils.h:
>
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/v8/src/utils.h&exact_package=chromium&q=file:v8/src/utils.h&l=300
> This is supposed to be accessed through the class below, Access, which
> is a friend of utils.h. Do you check if any friends use private
> variables?
>

Yes, I check friends. This is a bug, I know why it happens (it is because
of the dependent templates) and I will fix it.


> In chromium itself, it flags ShadowingAtExitManager member variables.
> This is basically a RAII class which only exists to call a protected
> superclass constructor, and which only exists if UNIT_TESTS is
> defined:
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/at_exit.h&l=71
> So maybe the "constructor without arguments" heuristic could be
> tweaked to exclude constructors that call superclass constructors with
> arguments?
> (Here's another RAII class whose constructor takes 0 arguments:
>
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/logging_unittest.cc&exact_package=chromium&q=logstatesaver&l=26
> )
>

I have changed it to be more conservative (I will send a new patch once I
have fixed the bug mentioned above). It now only accepts members without
initializer or with an argument-less initializer, if the field's type has a
trivial default constructor and a trivial destructor. Unfortunately, we now
also miss a lot of true positives, but I guess the false positives are
worse, especially because there is no easy way to explicitly suppress the
warning for these cases within the standard syntax.

Cheers,
Daniel


>
> (This is not an exhaustive list, just the things I found after a quick
> look.)
>
> Nico
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120112/f1face5e/attachment.html>