On Sat, Jul 27, 2013 at 4:45 PM, David Blaikie <[email protected]> wrote: > > On Jul 27, 2013 1:30 PM, "Aaron Ballman" <[email protected]> wrote: >> >> On Sat, Jul 27, 2013 at 4:09 PM, David Blaikie <[email protected]> wrote: >> > On Sat, Jul 27, 2013 at 12:50 PM, Aaron Ballman <[email protected]> >> > wrote: >> >> 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. >> > >> > Making the dtor explicit suppresses the warning - but it doesn't seem >> > obvious to any reader why the type has an explicit dtor. (& all it >> > does is defer the error to code that would call the dtor - which is >> > what MSVC should'e done in the first place, it seems) >> > >> >> 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). >> > >> > I don't believe this is a goal of the project. Having several >> > compilers all with different warnings makes it harder to maintain the >> > zero-warning bar on every one of them. >> >> Understandable, which is why I think it's fine that we disable >> warnings like C4800 (''type' : forcing value to bool 'true' or 'false' >> (performance warning)'); they don't add to the project. But at the >> same time, we disable warnings which we really should not, such as the >> recently re-enabled C4715 (''function' : not all control paths return >> a value'). > > Out of curiosity, does this fire in places where clang does not?
It did not appear to -- thankfully, there was no code causing that warning to trigger for MSVC. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
