[PATCH] Re-enabling another warning

[David Blaikie]

On Sat, Jul 27, 2013 at 12:50 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.

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. Generally the approach has been
to implement anything worthwhile in Clang and disable anything that's
not worthwhile. The end result being that Clang will generally warn on
anything we care about and Clang-warning-free code won't break the
build when using any other 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.

Perhaps I'm missing something, but it seems actually less clear - it's
not obvious why the dtor is explicit nor that sometimes the dtor is
invalid (due to the inaccessible dtor in the T member).