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

[Arnaud Bienner via cfe-dev]

Thinking again about this, I wonder if having this warning implemented, off
by default (since Aaron's proposal is not implemented yet) couldn't be a
good idea anyway.
I was initially convinced by Aaron that adding default-off warnings might
not be interesting, since not many people are likely to use it.
But I realized I will not be the only one to use it: the C++ coding rules
we are using at the company I'm working in forbid to call virtual functions
in constructors/destructors, and I realized Google's C++ styleguide (which
as far as I know I also used by many non-Google projects) also forbids
this:
https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
(maybe some Google C++ developers here can comment on how strictly those
guidelines are enforced in their projects).
CppCoreGuidelines also discourage this:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-ctor-virtual

Having a warning to check this behavior (with -Werror) would help ensure
the code conforms to the coding style and force developers to follow this
rule.

With this in mind, do you think we can reconsider having such a warning
implemented in clang?

Arnaud


Le lun. 14 janv. 2019 à 20:49, Aaron Ballman <aaron at aaronballman.com> a
écrit :

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190701/1f45a49d/attachment.html>