[PATCH] Re-enabling another warning

[Aaron Ballman]

On Sat, Jul 27, 2013 at 4:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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.

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').

> 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.

Which is understandable and why I am not suggesting we enable /WX
(treat warnings as errors in MSVC).  But at the same time, I feel the
bar should be a bit higher than it's been in the past for disabling
warnings across all files in MSVC.

>> 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).

This is sufficient for me to drop the patches.  I was testing the
waters to see if others felt it was more or less clear, and since
there's contention, I'm happy to leave it be.

~Aaron