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

[Arnaud Bienner via cfe-dev]

So I wrote a patch to diagnose simple cases like B.
https://reviews.llvm.org/D56366
I think most complicated cases should be catch by a static analyzer indeed,
especially since they might be harder to fix (if the you call a function
'f' from the constructor, and 'f' calls a virtual function: what if if 'f'
is also called after the object has been created?)
Cases like B are easy to fix, and only benefit from being fixed, as noted
earlier.

I run my patched-clang on the Firefox codebase, without any occurrence of
the warning. I guess that might be because calling virtual functions from
constructors/destructors is likely to create issues (like said previously),
and so is not common in this codebase.

If someone wants to try this patch on another codebase and share some
feedback, I would be curious to know how it goes!
Or if you have suggestion about some additional open source projects I
could try building with this new warning, I'm interested too.

Arnaud


Le mer. 19 déc. 2018 à 15:22, Aaron Ballman <aaron at aaronballman.com> a
écrit :

> On Tue, Dec 18, 2018 at 6:59 PM Artem Dergachev via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> >
> > Static Analyzer's checker already has a mode in which it warns on case
> B, but it's off by default because it is surprisingly noisy. In particular,
> i vaguely recall that i've seen ~90 warnings on the OpenOffice codebase -
> none of them were pure virtual calls and most of them looked like false
> positives in the sense that the virtual function is called intentionally.
> Which means that i would be generally worried about this warning. That
> said, i don't know how warnings are accepted into Clang and whether there
> is a threshold on intentional violations. I suspect that it depends on
> whether the warning is expected to be on by default or be part of -Wall or
> only -Weverything.
>
> In general, warnings in Clang are expected to be on-by-default and
> have an extremely low false positive rate (certainly lower than for
> the static analyzer). I suspect this warning would not meet the usual
> bar for inclusion as a Clang warning given the number of false
> positives it's shown in the past. I think the static analyzer is a
> better place for the functionality to live, especially given that it
> requires actual static analysis of runtime control flows to reduce
> those false positives.
>
> > Another good thing to do with a warning that may be intentionally
> triggered by the user is to add a hint on how to suppress it. Eg., how
> -Wparentheses does it:
> >
> >   test.c:2:9: note: place parentheses around the assignment to silence
> this warning
> >     if (x = 1) {
> >           ^
> >         (    )
> >
> > I guess you could add a note that suggests to qualify the call with a
> class name, which is a good thing to do anyway. With that, in my opinion
> it'd be a good warning.
>
> Agreed that this would be a sensible way for users to suppress the
> diagnostic -- it clarifies that the programmer understands the
> behavior of the call.
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190106/a42365c7/attachment.html>