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

[Aaron Ballman via cfe-dev]

On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <arnaud.bienner at gmail.com> wrote:
>
> 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?

I agree that there is utility in the check and that people will want
something like this. CERT also has a secure coding rule covering this
(https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors).

However, we have the optin.cplusplus.VirtualCall check already
implemented in the static analyzer which should meet all those needs
without adding an off-by-default diagnostic to the compiler. I've not
seen much discussion on why we shouldn't be improving that check
instead.

~Aaron

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