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

[Aaron Ballman via cfe-dev]

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

~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