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

[Daniel Jasper]

>
> First off, what do people think about turning this on by default, or at
> least making it part of -Wunused? Daniel's being conservative by leaving it
> off, but you know what people say about off-by-default warnings...
>

It would be my intention to turn it on by default (not sure whether by
default, as part of Wunused or just as default for LLVM compilations), but
I though we should clean up LLVM's codebase before doing so. But I am not
sure what the right approach is here.

---
>
> As for the patch…sorry, one last problem.
>
> +    // Mark as fully defined for now to stop recursion in case of mutual
> +    // friends.
> +    FullyDefined.insert(RD);
>
> I thought about this some more, and it seems like this will cause
> incorrect warnings:
>
> class B;
> class A {
>   int x;
> public:
>   friend class B;
>   doSomethingWithX();
> }
>
> class B {
>   int y;
> public:
>   friend class A;
> }
>
> When looking through A's decls (because of A::x), we'll come across
> "friend class B". We'll look through B's decls and find "friend class A". A
> has already been marked complete, so we mark B complete. Then we go back to
> A and find "doSomethingWithX()", and A is now (correctly and finally)
> marked incomplete. When we get to B::y, though, we're in trouble, because B
> has already been marked as complete via A!
>
> The usual thing to do here is add an "in progress" set, but I'm still not
> sure exactly how to use that for warnings. I think the best thing to do
> (and possibly the most correct thing to do) is to simply give up when we
> see mutual friends declarations -- that is, tentatively mark classes as not
> fully defined instead of fully defined. That gives us a conservative set of
> warnings, even if we miss a few valid cases.
>

Thanks for coming up with this!! I propose a different solution which
utilizes the fact that friend relationships are not transitive. Thus, if a
class has a friend, it only needs to check whether all methods and nested
classes of the friend are defined but does not need to consider the
friend's friends. I have changed the implementation and added corresponding
test cases (see attached new patch).

> Finally, even though there's not a big hit traversing the fields at the
>> end, we probably still shouldn't pay for this if the diagnostic is off. Can
>> you check for that before inserting into UnusedPrivateFields, or even doing
>> all the tests?
>>
>
> I am now checking whether the diagnostic is enabled before inserting into
> the set and before doing the checks at the end of the translation unit.
> Checking before removing from the (then empty) set probably does not give
> any benefit. I am not sure whether the check on the initializer arguments
> is expensive (especially the call to HasSideEffects). What do you think?
>
>
> Let's leave it as is. Looking up a diagnostic isn't /that/ cheap, and most
> initializers are something simple like a VarDecl, a constant, or a
> FunctionCall.
>

Ok.

I think once you've fixed the friend problem you can go ahead and commit.
> Thanks for working on this!
> Jordan
>

Could you take one last look as the implementation of IsRecordFullyDefined
has changed substantially?

Thank you!
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120604/6e8d2c27/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unused-private-field.patch
Type: application/octet-stream
Size: 15109 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120604/6e8d2c27/attachment.obj>