Warning when calling virtual functions from constructor/desctructor?

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
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
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 <[hidden email]> 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 <[hidden email]> 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 <
>> [hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev
<[hidden email]> 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.

~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 <[hidden email]> 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 <[hidden email]> 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 <
> >> [hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On 14 Jan 2019, at 14:23, Aaron Ballman wrote:

> On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev
> <[hidden email]> 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.

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 <[hidden email]> 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 <[hidden email]> 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 <
>>>> [hidden email]> 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
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
> > <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <
> >>>> [hidden email]> 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
> >> [hidden email]
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On 14 Jan 2019, at 14:40, Aaron Ballman wrote:

> On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
>>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>>>>>> 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 <
>>>>>> [hidden email]> 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
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:

>
> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
> > On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
> >>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> >>>>>> 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 <
> >>>>>> [hidden email]> 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
> >>>> [hidden email]
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann 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).

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 <[hidden email]> a écrit :
On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:
>
> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
> > On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
> >>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> >>>>>> 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 <
> >>>>>> [hidden email]> 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
> >>>> [hidden email]
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <[hidden email]> 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 <[hidden email]> a écrit :
>>
>> On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:
>> >
>> > On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
>> > > On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
>> > >>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>> > >>>>>> 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 <
>> > >>>>>> [hidden email]> 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
>> > >>>> [hidden email]
>> > >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev

On 1 Jul 2019, at 11:23, Aaron Ballman wrote:

On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <[hidden email]> 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 <[hidden email]> a écrit :

On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:

On 14 Jan 2019, at 14:40, Aaron Ballman wrote:

On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
<[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
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 <
[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
"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
[...] it's just generally better to
diagnose things in the compiler when possible"
Exactly: from my experience, static analysis is performed at the commit stage/during CI.
Having the possibility to catch issues ealier, as developers are writing the code (through Werror) can save a lot of time (this is why I'm insisting a bit on getting this into the compiler :) )

Arnaud


Le lun. 1 juil. 2019 à 19:37, John McCall <[hidden email]> a écrit :

On 1 Jul 2019, at 11:23, Aaron Ballman wrote:

On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <[hidden email]> 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 <[hidden email]> a écrit :

On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:

On 14 Jan 2019, at 14:40, Aaron Ballman wrote:

On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
<[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
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 <
[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On Mon, Jul 1, 2019 at 4:05 PM Arnaud Bienner <[hidden email]> wrote:
>
> "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

I'm not certain I agree with this approach in general because it
introduces unfortunate interactions between the static analyzer and
the compiler that make the user's life worse in practice. Will the
static analyzer produce duplicate diagnostics? What if the user
disables the compiler warning with a -Wno- flag, does the static
analyzer then go back to reporting the otherwise-duplicate warnings?
Does enabling the check in the static analyzer disable the compiler
warning in favor of the more accurate reports from the analyzer? Do we
need to carefully maintain both checks such that there is no
overlapping diagnostics between them?

To me, if the check cannot be reliably implemented in the compiler, it
belongs somewhere else (static analyzer, clang-tidy, etc). If we want
to split diagnostics across multiple tools, I think we should have
more of a plan in place for how those tools coordinate reporting
diagnostics so the user doesn't have a poor experience.

> [...] it's just generally better to
> diagnose things in the compiler when possible"
> Exactly: from my experience, static analysis is performed at the commit stage/during CI.
> Having the possibility to catch issues ealier, as developers are writing the code (through Werror) can save a lot of time (this is why I'm insisting a bit on getting this into the compiler :) )

I definitely agree that we want to catch issues earlier rather than
later when possible. What I don't think is a good idea is having two
different ways to catch the same issues, where one of the ways will
have a higher false negative rate compared to the other.

~Aaron

>
> Arnaud
>
>
> Le lun. 1 juil. 2019 à 19:37, John McCall <[hidden email]> a écrit :
>>
>> On 1 Jul 2019, at 11:23, Aaron Ballman wrote:
>>
>> On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <[hidden email]> 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 <[hidden email]> a écrit :
>>
>> On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]> wrote:
>>
>> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
>>
>> On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]> 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
>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>> 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 <
>> [hidden email]> 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
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning when calling virtual functions from constructor/desctructor?

Kristóf Umann via cfe-dev
On 1 Jul 2019, at 16:24, Aaron Ballman wrote:

> On Mon, Jul 1, 2019 at 4:05 PM Arnaud Bienner
> <[hidden email]> wrote:
>>
>> "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
>
> I'm not certain I agree with this approach in general because it
> introduces unfortunate interactions between the static analyzer and
> the compiler that make the user's life worse in practice. Will the
> static analyzer produce duplicate diagnostics? What if the user
> disables the compiler warning with a -Wno- flag, does the static
> analyzer then go back to reporting the otherwise-duplicate warnings?
> Does enabling the check in the static analyzer disable the compiler
> warning in favor of the more accurate reports from the analyzer? Do we
> need to carefully maintain both checks such that there is no
> overlapping diagnostics between them?
>
> To me, if the check cannot be reliably implemented in the compiler, it
> belongs somewhere else (static analyzer, clang-tidy, etc). If we want
> to split diagnostics across multiple tools, I think we should have
> more of a plan in place for how those tools coordinate reporting
> diagnostics so the user doesn't have a poor experience.

There are dozens of problems that we warn about in the compiler for
direct violations and in the static analyzer for more complex cases:
uninitialized memory, dereferencing null, pure-virtual calls in
constructors, and so on.  Nothing about these compiler warnings is
"unreliable"; they just have false negatives (as does the static
analyzer, of course).

John.

>
>> [...] it's just generally better to
>> diagnose things in the compiler when possible"
>> Exactly: from my experience, static analysis is performed at the
>> commit stage/during CI.
>> Having the possibility to catch issues ealier, as developers are
>> writing the code (through Werror) can save a lot of time (this is why
>> I'm insisting a bit on getting this into the compiler :) )
>
> I definitely agree that we want to catch issues earlier rather than
> later when possible. What I don't think is a good idea is having two
> different ways to catch the same issues, where one of the ways will
> have a higher false negative rate compared to the other.
>
> ~Aaron
>
>>
>> Arnaud
>>
>>
>> Le lun. 1 juil. 2019 à 19:37, John McCall <[hidden email]> a
>> écrit :
>>>
>>> On 1 Jul 2019, at 11:23, Aaron Ballman wrote:
>>>
>>> On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner
>>> <[hidden email]> 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
>>> <[hidden email]> a écrit :
>>>
>>> On Mon, Jan 14, 2019 at 2:44 PM John McCall <[hidden email]>
>>> wrote:
>>>
>>> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
>>>
>>> On Mon, Jan 14, 2019 at 2:31 PM John McCall <[hidden email]>
>>> 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
>>> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>>> 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 <
>>> [hidden email]> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12