On Mon, Jun 4, 2012 at 10:42 PM, Richard Smith <[email protected]>wrote:
> On Mon, Jun 4, 2012 at 12:36 PM, Daniel Jasper <[email protected]> wrote: > > On Mon, Jun 4, 2012 at 8:33 PM, Richard Smith <[email protected]> > wrote: > >> I have a few comments in addition to Jordy's: > >> > >> * Since pure virtual functions can be defined, the diagnostic as it > >> stands might have false positives. In general, I think that's > >> unavoidable and acceptable, but in the specific case of a pure virtual > >> destructor, I think we should require that the TU contains a > >> definition: such a destructor must actually be defined for the class > >> to be useful as a base class, and may well be the only member of the > >> class which has an out-of-line definition, and might reasonably > >> contain the only use (other than a side-effect-free initialization) of > >> a private member. > > > > > > Do you mean that we should require that as part of this warning (i.e. > mark > > those classes as incomplete) or do you mean we should create a separate > > warning for that? > > I mean that we should not consider a class to be completely defined if > it has a pure virtual destructor with no definition in this TU. > Done and test added. > Also, do you think such a destructor might actually have > > side-effects based on the otherwise unused fields. I somewhat doubt that > as > > we are only looking at pointers or primitive fields.. We might have > > something like an owning pointer, but there the destructor will never be > the > > only "use". > > I'm imagining a case like this: > > // .h > class RAIIBase { > public: > RAIIBase(const char *p) : LogMessage(p) { > my_debug_stream << "Creating " << p; > } > virtual ~RAIIBase() = 0; > private: > const char *LogMessage; > }; > > // .cc > RAIIBase::~RAIIBase() { > my_debug_stream << "Destroying " << LogMessage; > } > I wonder, whether this is a case that really ever occurs (due to multiple rare things being used at the same time). But ok. > If I've understood correctly, the initialization of LogMessage won't > count as a 'use' because it has no side-effects, so any TU including > the .h file (other than the corresponding .cc file) would flag > 'LogMessage' as being unused. > > >> * It looks like in-class initializers and (in-constructor) member > >> initializers are handled slightly differently (particularly for > >> non-class types where the initializer has side-effects). It would seem > >> more consistent with the language semantics if in-class initializers > >> were treated exactly like member initializers, and only considered if > >> they're actually used by a constructor. [... it also strikes me that a > >> warning for an unused in-class initializer could be useful :) ...] > > > > > > Just to clarify: An in-class initializer is only used, if at least one > > constructor does not have a corresponding member initializer? > > There are some additional complications if the class is a union or has > an anonymous union member. Perhaps moving the marking-as-used for > in-class initializers to CollectFieldInitializer would be best. > Moved this to CollectFieldInitializer. Added call to HasSideEffects (+test) for symmetry with constructor initializers. > > >> * I would like to see some test cases for unions, and for anonymous > >> struct/union members, in particular for cases like this: > >> > >> class S { > >> // ... no use of Aligner ... > >> private: > >> union { > >> void *Aligner; > >> unsigned char Data[8]; > >> }; > >> }; > > > > > > I'd like to get the first version in. Thus I'd like to add this test case > > together with a FIXME. The correct solution for this (and especially for > > anonymous structs) would be to look at the corresponding DeclContext, > right? > > Yes, to find the outer struct, you can walk through the parent > DeclContexts (while they're RecordDecls and > RecordDecl::isAnonymousStructOrUnion returns true). Ok, I will do this once the first patch is in as I am not overly concerned about this kind of false negative. > >> (See clang::DependentFunctionTempateSpecializationInfo for a > >> real-world example of this.) It looks like we won't diagnose that case > >> (because Aligner is technically public, despite being effectively a > >> private member of S). But I suspect we will diagnose this (and > >> probably shouldn't): > >> > >> union S { > >> // ... no use of Aligner ... > >> private: > >> void *Aligner; > >> unsigned char Data[8]; > >> }; > >> > >> Perhaps we should conservatively disable the diagnostic for union > members? > > > > > > Why should we not warn in this case? I think in general we might want to > > exempt char-arrays as they are commonly use for alignment, but other than > > that? > > Aligner is 'unused', but has a semantic effect. Note that in the > clang::DependentFunctionTempateSpecializationInfo case, there is no > char array. I'm concerned that a warning for cases like this would > have a high false positive rate. (I have the same concern for padding > members in structs, but I would expect those to generally be public, > since such structs are usually intended to be PODs.) > Ah, (things-I-know-about-C++)++. :-) I know exclude unions from this analysis. New patch attached.
unused-private-field.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
