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

[John McCall via cfe-dev]

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.

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