<p dir="ltr"><br>
On Jul 27, 2013 1:30 PM, "Aaron Ballman" <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
><br>
> On Sat, Jul 27, 2013 at 4:09 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > On Sat, Jul 27, 2013 at 12:50 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> >> On Sat, Jul 27, 2013 at 3:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >>> On Sat, Jul 27, 2013 at 12:27 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> >>>> On Sat, Jul 27, 2013 at 2:59 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >>>>> On Sat, Jul 27, 2013 at 11:48 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> >>>>>> This patch fixes a few cases where a default destructor would not be<br>
> >>>>>> generated due to a member variable having an inaccessible default<br>
> >>>>>> destructor. While the original code was correct, the updated code is<br>
> >>>>>> a bit more clear. Assuming that this new code is acceptable, this<br>
> >>>>>> also re-enables the C4624 MSVC warning (''derived class' : destructor<br>
> >>>>>> could not be generated because a base class destructor is<br>
> >>>>>> inaccessible').<br>
> >>>>><br>
> >>>>> That code change does seem a bit strange/questionable/unnecessary.<br>
> >>>><br>
> >>>> This is why I was passing it by the lists. ;-)<br>
> >>>><br>
> >>>>> While I'm OK enabling warnings in MSVC that have some substantial<br>
> >>>>> overlap with Clang warnings to help developers working with MSVC keep<br>
> >>>>> their work Clang-clean, generally we've taken the line that if a<br>
> >>>>> warning isn't viable for Clang, it's not worth turning on in some<br>
> >>>>> other compiler for LLVM. Does this warning catch useful bugs? (Why<br>
> >>>>> would you need a warning for cases when a dtor couldn't be generated<br>
> >>>>> when you'd get an error if/when you actually call that dtor? (because<br>
> >>>>> that latter warning is hard to understand? That would be unfortunate))<br>
> >>>><br>
> >>>> I think the benefit is that it points out when destructors are not<br>
> >>>> generated, which isn't always obvious.<br>
> >>><br>
> >>> Why would it matter if the dtor is never called? If it is called, I<br>
> >>> imagine you get an actual error, no?<br>
> >><br>
> >> You would, but the error isn't likely to be as obvious as it is with<br>
> >> an explicit destructor (at least in MSVC).<br>
> >><br>
> >>>>> I wonder why making an explicit dtor works? If the base class dtor is<br>
> >>>>> inaccessible, wouldn't it still be inaccessible even in an explicit<br>
> >>>>> dtor?<br>
> >>>><br>
> >>>> The warning is about generating the implicit destructor for the<br>
> >>>> derived class. By providing an explicit destructor for the derived<br>
> >>>> class, the implicit one cannot be generated so the warning doesn't<br>
> >>>> apply.<br>
> >>><br>
> >>> But either explicit or implicit, the dtor has to call the base dtor -<br>
> >>> why would it be accessible when explicit but inaccessible when<br>
> >>> implicit?<br>
> >><br>
> >> The base class destructor would remain inaccessible.<br>
> >><br>
> >>> OK, now I've looked at the code I'm even more confused -<br>
> >>> AlignmentCalcImpl has no base classes nor do any classes derive from<br>
> >>> it. So which base/derived classes is this diagnostic referring to?<br>
> >><br>
> >> The warning isn't particularly well-worded IMO. The problem with<br>
> >> AlignmentCalcImpl is the T -- for some values of T, the destructor is<br>
> >> inaccessible. Since there's a value type of T in the class with an<br>
> >> inaccessible destructor, AlignmentCalcImpl does not get an implicit<br>
> >> destructor either.<br>
> ><br>
> > Making the dtor explicit suppresses the warning - but it doesn't seem<br>
> > obvious to any reader why the type has an explicit dtor. (& all it<br>
> > does is defer the error to code that would call the dtor - which is<br>
> > what MSVC should'e done in the first place, it seems)<br>
> ><br>
> >> I don't feel incredibly strongly about this particular warning, but I<br>
> >> think we should strive to have as few warnings turned off by default<br>
> >> as possible (regardless of compiler).<br>
> ><br>
> > I don't believe this is a goal of the project. Having several<br>
> > compilers all with different warnings makes it harder to maintain the<br>
> > zero-warning bar on every one of them.<br>
><br>
> Understandable, which is why I think it's fine that we disable<br>
> warnings like C4800 (''type' : forcing value to bool 'true' or 'false'<br>
> (performance warning)'); they don't add to the project. But at the<br>
> same time, we disable warnings which we really should not, such as the<br>
> recently re-enabled C4715 (''function' : not all control paths return<br>
> a value').</p>
<p dir="ltr">Out of curiosity, does this fire in places where clang does not?</p>
<p dir="ltr">> > Generally the approach has been<br>
> > to implement anything worthwhile in Clang and disable anything that's<br>
> > not worthwhile. The end result being that Clang will generally warn on<br>
> > anything we care about and Clang-warning-free code won't break the<br>
> > build when using any other compiler.<br>
><br>
> Which is understandable and why I am not suggesting we enable /WX<br>
> (treat warnings as errors in MSVC). But at the same time, I feel the<br>
> bar should be a bit higher than it's been in the past for disabling<br>
> warnings across all files in MSVC.<br>
><br>
> >> This one isn't as egregious as<br>
> >> some others I've re-enabled recently, but I do still think it can make<br>
> >> the code slightly more clear when being explicit.<br>
> ><br>
> > Perhaps I'm missing something, but it seems actually less clear - it's<br>
> > not obvious why the dtor is explicit nor that sometimes the dtor is<br>
> > invalid (due to the inaccessible dtor in the T member).<br>
><br>
> This is sufficient for me to drop the patches. I was testing the<br>
> waters to see if others felt it was more or less clear, and since<br>
> there's contention, I'm happy to leave it be.</p>
<p dir="ltr">Sure, though I'd be happy to hear an explanation if I'm missing something or misrepresenting the issue. In any case, others can always speak up and present other perspectives if we've not covered then</p>
<p dir="ltr">><br>
> ~Aaron<br>
</p>