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

[Arnaud Bienner 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 <jmccall at apple.com> a écrit :

> On 1 Jul 2019, at 11:23, Aaron Ballman wrote:
>
> On Mon, Jul 1, 2019 at 10:36 AM Arnaud Bienner <arnaud.bienner at gmail.com>
> wrote:
>
> Thinking again about this, I wonder if having this warning implemented,
> off by default (since Aaron's proposal is not implemented yet) couldn't be
> a good idea anyway.
> I was initially convinced by Aaron that adding default-off warnings might
> not be interesting, since not many people are likely to use it.
> But I realized I will not be the only one to use it: the C++ coding rules
> we are using at the company I'm working in forbid to call virtual functions
> in constructors/destructors, and I realized Google's C++ styleguide (which
> as far as I know I also used by many non-Google projects) also forbids
> this:
> https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
> (maybe some Google C++ developers here can comment on how strictly those
> guidelines are enforced in their projects).
> CppCoreGuidelines also discourage this:
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-ctor-virtual
>
> Having a warning to check this behavior (with -Werror) would help ensure
> the code conforms to the coding style and force developers to follow this
> rule.
>
> With this in mind, do you think we can reconsider having such a warning
> implemented in clang?
>
> I agree that there is utility in the check and that people will want
> something like this. CERT also has a secure coding rule covering this
> (
> https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors
> ).
>
> However, we have the optin.cplusplus.VirtualCall check already
> implemented in the static analyzer which should meet all those needs
> without adding an off-by-default diagnostic to the compiler. I've not
> seen much discussion on why we shouldn't be improving that check
> instead.
>
> Abstractly, I think it makes sense to diagnose the obvious case of this
> in the compiler and let the static analyzer catch the less-obvious cases,
> like when the analysis has to be interprocedural. That's a standard
> trade-off we make for a lot of diagnostics: it's just generally better to
> diagnose things in the compiler when possible.
>
> If we take that as given, then the question is just whether our general
> policy of "(mostly) no new opt-in warnings" should have an exception for
> warnings that we'd like to be opt-out except for the lack of something
> like -Wversion.
>
> John.
>
> Le lun. 14 janv. 2019 à 20:49, Aaron Ballman <aaron at aaronballman.com> a
> écrit :
>
> On Mon, Jan 14, 2019 at 2:44 PM John McCall <jmccall at apple.com> wrote:
>
> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:
>
> On Mon, Jan 14, 2019 at 2:31 PM John McCall <jmccall at apple.com> wrote:
>
> On 14 Jan 2019, at 14:23, Aaron Ballman wrote:
>
> On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>
> On 11 Jan 2019, at 11:23, Arnaud Bienner wrote:
>
> There's a fairly substantial difference between warnings that
> target
>
> patterns
>
> that are *inarguably* bad, like the std::move problem (which in
> some
>
> sense is
>
> a language defect that people just need to be taught how to work
> around),
>
> and
>
> warnings that target code patterns that might be confusing or
> which
> have a
> higher-than-normal chance of being a typo. -Wparentheses is the
> classic
> example here
>
> Yes, and this warning is definitely like -Wparentheses: something
> that
> could be wrong, but is not necessarily.
> Still, I think it will catch tricky bugs.
>
> I agree.
>
> What should we do about this warning/patch?
> Deactivate it by default, with its own flag? without putting it in
> Wall/extra?
> Or wait for something like your proposal to be implemented?
> Or just forget about it for now and simply wait for more feedback
> from
> the
> mailing before deciding?
>
> I think it should go in, default-off, and we should implement my
> proposal
>
> AIUI, experience has shown that off-by-default warning flags almost
> never get enabled by users. I don't think we should have a new,
> default off warnings unless there is a strong use case suggesting
> someone will actually enable it.
>
> See my proposal earlier in the thread for how we can evolve the set
> of
> "always"-on warnings more aggressively without breaking source
> compatibility.
>
> I saw it (and think it's a good proposal), but without something like
> that, this diagnostic is unlikely to ever be enabled by default, so
> why should we adopt it when there are better near-term alternatives
> for the feature (such as improving the static analyzer)?
>
> Okay, so your argument is that we should try moving forward with that
> proposal as a pre-requisite to adding warnings that would otherwise have
> to be default-off? That seems reasonable.
>
> Yes, that would be my preference. It's a more conservative approach,
> but I think that's not too bad given that the warnings would otherwise
> be off-by-default anyway.
>
> ~Aaron
>
> John.
>
> ~Aaron
>
> John.
>
> ~Aaron
>
> as a way of eventually making it default-on. Unfortunately, I
> don't
> really
> have time to implement my proposal right now; I'm way backed up as
> it
> is.
>
> John.
>
> Arnaud
>
>
>
>
>
> Le mer. 9 janv. 2019 à 01:14, John McCall <jmccall at apple.com> a
> écrit :
>
> On 8 Jan 2019, at 15:03, Arthur O'Dwyer wrote:
>
> I was thinking of `-Wreturn-std-move`, which is
> -Wmove/-Wmost/-Wall
> but not
> always-on.
>
> I honestly don't know why this isn't default-on.
>
> Grepping the code for DefaultIgnore, I see that
> -Wfor-loop-analysis
> is
> another example (but 4 years old).
>
> This is a harder call. At any rate, I think my point stands that
> this
> is
> not
> "frequent".
>
> There's a fairly substantial difference between warnings that
> target
> patterns
> that are *inarguably* bad, like the std::move problem (which in
> some
> sense is
> a language defect that people just need to be taught how to work
> around),
> and
> warnings that target code patterns that might be confusing or
> which
> have a
> higher-than-normal chance of being a typo. -Wparentheses is the
> classic
> example here: it unquestionably catches a common mistake that C
> unfortunately
> otherwise masks, but it's still perennially controversial because
> the
> assign-and-test idiom is so common in C programming, and there
> are a
> lot of
> people who still swear by reversing the equality test (0 == foo)
> instead
> of
> relying on the warning.
>
> John.
>
> On Tue, Jan 8, 2019 at 2:37 PM John McCall <jmccall at apple.com>
> wrote:
>
> On 8 Jan 2019, at 13:45, Arthur O'Dwyer wrote:
>
> On Tue, Jan 8, 2019 at 1:05 PM John McCall via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> On 8 Jan 2019, at 8:37, Arnaud Bienner wrote:
>
> I realized I didn't put "DefaultIgnore" on this warning.
> I'm not experienced enough with clang to know what's the best way
> to
> deal
> with new warnings, but my feeling is that it would be sensible to
> have
>
> this
>
> new warning DefaultIgnore for now, in -Wall, and make it default
> once we
> have some feedback from the community: while not all C++ projects
> use
>
> -Wall
>
> (or -Wextra) I believe enough do to give us a chance to get some
>
> feedback.
>
> What do you think?
>
> We generally don't add things to -Wall. That's why I went into my
> whole
> spiel
> about versioning: I think it's a conversation we need to have
> before
> we're
> ready to accept this as a warning that's anything but hidden
> permanently
> behind its own opt-in flag.
>
> John: Wha? Clang *frequently* adds things to -Wall! -Wall
> includes
> -Wmost
> which includes a bunch of other categories, so while we don't
> often
> put new
> diagnostics *directly* under -Wall, pretty much every
> "reasonable"
> diagnostic eventually winds up in there somehow — which is the
> intent.
>
> I don't think we "frequently" add things to -Wall or -Wmost. We
> do
> somewhat
> frequently add warnings that are unconditionally default-on, but
> the
> groups
> have a conventional meaning that we don't generally touch. What
> recently-added
> warnings are you thinking of that are not default-on but which
> are
> included
> in a group like -Wall or -Wmost?
>
> John.
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190701/55049dd8/attachment.html>