On Sat, Jul 27, 2013 at 3:40 PM, David Blaikie <[email protected]> wrote: > On Sat, Jul 27, 2013 at 12:27 PM, Aaron Ballman <[email protected]> > wrote: >> On Sat, Jul 27, 2013 at 2:59 PM, David Blaikie <[email protected]> wrote: >>> On Sat, Jul 27, 2013 at 11:48 AM, Aaron Ballman <[email protected]> >>> wrote: >>>> This patch fixes a few cases where a default destructor would not be >>>> generated due to a member variable having an inaccessible default >>>> destructor. While the original code was correct, the updated code is >>>> a bit more clear. Assuming that this new code is acceptable, this >>>> also re-enables the C4624 MSVC warning (''derived class' : destructor >>>> could not be generated because a base class destructor is >>>> inaccessible'). >>> >>> That code change does seem a bit strange/questionable/unnecessary. >> >> This is why I was passing it by the lists. ;-) >> >>> While I'm OK enabling warnings in MSVC that have some substantial >>> overlap with Clang warnings to help developers working with MSVC keep >>> their work Clang-clean, generally we've taken the line that if a >>> warning isn't viable for Clang, it's not worth turning on in some >>> other compiler for LLVM. Does this warning catch useful bugs? (Why >>> would you need a warning for cases when a dtor couldn't be generated >>> when you'd get an error if/when you actually call that dtor? (because >>> that latter warning is hard to understand? That would be unfortunate)) >> >> I think the benefit is that it points out when destructors are not >> generated, which isn't always obvious. > > Why would it matter if the dtor is never called? If it is called, I > imagine you get an actual error, no?
You would, but the error isn't likely to be as obvious as it is with an explicit destructor (at least in MSVC). >>> I wonder why making an explicit dtor works? If the base class dtor is >>> inaccessible, wouldn't it still be inaccessible even in an explicit >>> dtor? >> >> The warning is about generating the implicit destructor for the >> derived class. By providing an explicit destructor for the derived >> class, the implicit one cannot be generated so the warning doesn't >> apply. > > But either explicit or implicit, the dtor has to call the base dtor - > why would it be accessible when explicit but inaccessible when > implicit? The base class destructor would remain inaccessible. > OK, now I've looked at the code I'm even more confused - > AlignmentCalcImpl has no base classes nor do any classes derive from > it. So which base/derived classes is this diagnostic referring to? The warning isn't particularly well-worded IMO. The problem with AlignmentCalcImpl is the T -- for some values of T, the destructor is inaccessible. Since there's a value type of T in the class with an inaccessible destructor, AlignmentCalcImpl does not get an implicit destructor either. I don't feel incredibly strongly about this particular warning, but I think we should strive to have as few warnings turned off by default as possible (regardless of compiler). This one isn't as egregious as some others I've re-enabled recently, but I do still think it can make the code slightly more clear when being explicit. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
