<div dir="ltr"><div dir="ltr"><div>So I wrote a patch to diagnose simple cases like B.</div><div><a href="https://reviews.llvm.org/D56366">https://reviews.llvm.org/D56366</a></div><div>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?)</div><div>Cases like B are easy to fix, and only benefit from being fixed, as noted earlier.<br></div><div><br></div><div>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.<br></div><div><br></div><div>If someone wants to try this patch on another codebase and share some feedback, I would be curious to know how it goes!<br></div><div>Or if you have suggestion about some additional open source projects I could try building with this new warning, I'm interested too.<br></div><div><br></div><div>Arnaud</div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">Le mer. 19 déc. 2018 à 15:22, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Dec 18, 2018 at 6:59 PM Artem Dergachev via cfe-dev<br>
<<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
><br>
> 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.<br>
<br>
In general, warnings in Clang are expected to be on-by-default and<br>
have an extremely low false positive rate (certainly lower than for<br>
the static analyzer). I suspect this warning would not meet the usual<br>
bar for inclusion as a Clang warning given the number of false<br>
positives it's shown in the past. I think the static analyzer is a<br>
better place for the functionality to live, especially given that it<br>
requires actual static analysis of runtime control flows to reduce<br>
those false positives.<br>
<br>
> 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:<br>
><br>
>   test.c:2:9: note: place parentheses around the assignment to silence this warning<br>
>     if (x = 1) {<br>
>           ^<br>
>         (    )<br>
><br>
> 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.<br>
<br>
Agreed that this would be a sensible way for users to suppress the<br>
diagnostic -- it clarifies that the programmer understands the<br>
behavior of the call.<br>
<br>
~Aaron<br>
</blockquote></div>