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

[John McCall via cfe-dev]

On 1 Jul 2019, at 11:23, Aaron Ballman wrote:
> 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.

Abstractly, I think it makes sense to diagnose the obvious case of this
in the compiler and let the static analyzer catch the less-obvious 
cases,
like when the analysis has to be interprocedural.  That's a standard
trade-off we make for a lot of diagnostics: it's just generally better 
to
diagnose things in the compiler when possible.

If we take that as given, then the question is just whether our general
policy of "(mostly) no new opt-in warnings" should have an exception for
warnings that we'd like to be opt-out except for the lack of something
like `-Wversion`.

John.

>> 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/80abfe56/attachment.html>