[cfe-dev] Warning when calling virtual functions from constructor/desctructor?

[Aaron Ballman via cfe-dev]

On Mon, Jan 14, 2019 at 2:44 PM John McCall <jmccall at apple.com> wrote:
>
> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
> > On Mon, Jan 14, 2019 at 2:31 PM John McCall <jmccall at apple.com> wrote:
> >> On 14 Jan 2019, at 14:23, Aaron Ballman wrote:
> >>> On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev
> >>> <cfe-dev at lists.llvm.org> wrote:
> >>>>
> >>>> On 11 Jan 2019, at 11:23, Arnaud Bienner wrote:
> >>>>>> There's a fairly substantial difference between warnings that
> >>>>>> target
> >>>>> patterns
> >>>>>> that are *inarguably* bad, like the std::move problem (which in
> >>>>>> some
> >>>>> sense is
> >>>>>> a language defect that people just need to be taught how to work
> >>>>>> around),
> >>>>> and
> >>>>>> warnings that target code patterns that might be confusing or
> >>>>>> which
> >>>>>> have a
> >>>>>> higher-than-normal chance of being a typo. -Wparentheses is the
> >>>>>> classic
> >>>>>> example here
> >>>>>
> >>>>> Yes, and this warning is definitely like -Wparentheses: something
> >>>>> that
> >>>>> could be wrong, but is not necessarily.
> >>>>> Still, I think it will catch tricky bugs.
> >>>>
> >>>> I agree.
> >>>>
> >>>>> What should we do about this warning/patch?
> >>>>> Deactivate it by default, with its own flag? without putting it in
> >>>>> Wall/extra?
> >>>>> Or wait for something like your proposal to be implemented?
> >>>>> Or just forget about it for now and simply wait for more feedback
> >>>>> from
> >>>>> the
> >>>>> mailing before deciding?
> >>>>
> >>>> I think it should go in, default-off, and we should implement my
> >>>> proposal
> >>>
> >>> AIUI, experience has shown that off-by-default warning flags almost
> >>> never get enabled by users. I don't think we should have a new,
> >>> default off warnings unless there is a strong use case suggesting
> >>> someone will actually enable it.
> >>
> >> See my proposal earlier in the thread for how we can evolve the set
> >> of
> >> "always"-on warnings more aggressively without breaking source
> >> compatibility.
> >
> > I saw it (and think it's a good proposal), but without something like
> > that, this diagnostic is unlikely to ever be enabled by default, so
> > why should we adopt it when there are better near-term alternatives
> > for the feature (such as improving the static analyzer)?
>
> Okay, so your argument is that we should try moving forward with that
> proposal as a pre-requisite to adding warnings that would otherwise have
> to be default-off?  That seems reasonable.

Yes, that would be my preference. It's a more conservative approach,
but I think that's not too bad given that the warnings would otherwise
be off-by-default anyway.

~Aaron

>
> John.
>
> >
> > ~Aaron
> >
> >>
> >> John.
> >>
> >>>
> >>> ~Aaron
> >>>
> >>>> as a way of eventually making it default-on.  Unfortunately, I
> >>>> don't
> >>>> really
> >>>> have time to implement my proposal right now; I'm way backed up as
> >>>> it
> >>>> is.
> >>>>
> >>>> John.
> >>>>
> >>>>
> >>>>>
> >>>>> Arnaud
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Le mer. 9 janv. 2019 à 01:14, John McCall <jmccall at apple.com> a
> >>>>> écrit :
> >>>>>
> >>>>>> On 8 Jan 2019, at 15:03, Arthur O'Dwyer wrote:
> >>>>>>
> >>>>>> I was thinking of `-Wreturn-std-move`, which is
> >>>>>> -Wmove/-Wmost/-Wall
> >>>>>> but not
> >>>>>> always-on.
> >>>>>>
> >>>>>> I honestly don't know why this isn't default-on.
> >>>>>>
> >>>>>> Grepping the code for DefaultIgnore, I see that
> >>>>>> -Wfor-loop-analysis
> >>>>>> is
> >>>>>> another example (but 4 years old).
> >>>>>>
> >>>>>> This is a harder call. At any rate, I think my point stands that
> >>>>>> this
> >>>>>> is
> >>>>>> not
> >>>>>> "frequent".
> >>>>>>
> >>>>>> There's a fairly substantial difference between warnings that
> >>>>>> target
> >>>>>> patterns
> >>>>>> that are *inarguably* bad, like the std::move problem (which in
> >>>>>> some
> >>>>>> sense is
> >>>>>> a language defect that people just need to be taught how to work
> >>>>>> around),
> >>>>>> and
> >>>>>> warnings that target code patterns that might be confusing or
> >>>>>> which
> >>>>>> have a
> >>>>>> higher-than-normal chance of being a typo. -Wparentheses is the
> >>>>>> classic
> >>>>>> example here: it unquestionably catches a common mistake that C
> >>>>>> unfortunately
> >>>>>> otherwise masks, but it's still perennially controversial because
> >>>>>> the
> >>>>>> assign-and-test idiom is so common in C programming, and there
> >>>>>> are a
> >>>>>> lot of
> >>>>>> people who still swear by reversing the equality test (0 == foo)
> >>>>>> instead
> >>>>>> of
> >>>>>> relying on the warning.
> >>>>>>
> >>>>>> John.
> >>>>>>
> >>>>>> On Tue, Jan 8, 2019 at 2:37 PM John McCall <jmccall at apple.com>
> >>>>>> wrote:
> >>>>>>
> >>>>>> On 8 Jan 2019, at 13:45, Arthur O'Dwyer wrote:
> >>>>>>
> >>>>>> On Tue, Jan 8, 2019 at 1:05 PM John McCall via cfe-dev <
> >>>>>> cfe-dev at lists.llvm.org> wrote:
> >>>>>>
> >>>>>> On 8 Jan 2019, at 8:37, Arnaud Bienner wrote:
> >>>>>>
> >>>>>> I realized I didn't put "DefaultIgnore" on this warning.
> >>>>>> I'm not experienced enough with clang to know what's the best way
> >>>>>> to
> >>>>>> deal
> >>>>>> with new warnings, but my feeling is that it would be sensible to
> >>>>>> have
> >>>>>>
> >>>>>> this
> >>>>>>
> >>>>>> new warning DefaultIgnore for now, in -Wall, and make it default
> >>>>>> once we
> >>>>>> have some feedback from the community: while not all C++ projects
> >>>>>> use
> >>>>>>
> >>>>>> -Wall
> >>>>>>
> >>>>>> (or -Wextra) I believe enough do to give us a chance to get some
> >>>>>>
> >>>>>> feedback.
> >>>>>>
> >>>>>> What do you think?
> >>>>>>
> >>>>>> We generally don't add things to -Wall. That's why I went into my
> >>>>>> whole
> >>>>>> spiel
> >>>>>> about versioning: I think it's a conversation we need to have
> >>>>>> before
> >>>>>> we're
> >>>>>> ready to accept this as a warning that's anything but hidden
> >>>>>> permanently
> >>>>>> behind its own opt-in flag.
> >>>>>>
> >>>>>> John: Wha? Clang *frequently* adds things to -Wall! -Wall
> >>>>>> includes
> >>>>>> -Wmost
> >>>>>> which includes a bunch of other categories, so while we don't
> >>>>>> often
> >>>>>> put new
> >>>>>> diagnostics *directly* under -Wall, pretty much every
> >>>>>> "reasonable"
> >>>>>> diagnostic eventually winds up in there somehow — which is the
> >>>>>> intent.
> >>>>>>
> >>>>>> I don't think we "frequently" add things to -Wall or -Wmost. We
> >>>>>> do
> >>>>>> somewhat
> >>>>>> frequently add warnings that are unconditionally default-on, but
> >>>>>> the
> >>>>>> groups
> >>>>>> have a conventional meaning that we don't generally touch. What
> >>>>>> recently-added
> >>>>>> warnings are you thinking of that are not default-on but which
> >>>>>> are
> >>>>>> included
> >>>>>> in a group like -Wall or -Wmost?
> >>>>>>
> >>>>>> John.
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> cfe-dev mailing list
> >>>> cfe-dev at lists.llvm.org
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev