[PATCH] Re-enabling another warning

[Aaron Ballman]

On Sat, Jul 27, 2013 at 3:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Sat, Jul 27, 2013 at 12:27 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Sat, Jul 27, 2013 at 2:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Sat, Jul 27, 2013 at 11:48 AM, Aaron Ballman <aaron at aaronballman.com> 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