[PATCH] Re-enabling another warning

[Aaron Ballman]

On Sat, Jul 27, 2013 at 4:45 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Jul 27, 2013 1:30 PM, "Aaron Ballman" <aaron at aaronballman.com> wrote:
>>
>> 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').
>
> 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