<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8">
</head>
<body>
<div style="font-family:sans-serif"><div style="white-space:normal">
<p dir="auto">On 19 Dec 2018, at 9:22, Aaron Ballman via cfe-dev wrote:</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">On Tue, Dec 18, 2018 at 6:59 PM Artem Dergachev via cfe-dev<br>
<cfe-dev@lists.llvm.org> wrote:</p>
<blockquote style="border-left:2px solid #777; color:#999; margin:0 0 5px; padding-left:5px; border-left-color:#999"><p dir="auto">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.</p>
</blockquote><p dir="auto">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>
</p>
<blockquote style="border-left:2px solid #777; color:#999; margin:0 0 5px; padding-left:5px; border-left-color:#999"><p dir="auto">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.</p>
</blockquote><p dir="auto">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.</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">Agreed.</p>
<p dir="auto">In general, whether or not to add a new default-on warning comes down to two<br>
somewhat orthogonal considerations:</p>
<ul>
<li><p dir="auto">Are we willing to say that the code pattern is Wrong?</p>
<p dir="auto">This involves weighing the harm of unknowing uses of the code pattern<br>
against the harm of avoiding the diagnostic.</p></li>
<li><p dir="auto">Are knowing, correct uses of the code pattern too common in practice<br>
to justify annoying a large fraction of the community?</p>
<p dir="auto">This involves weighing the diagnostic's impact on all code, which is<br>
impossible, so we have to muddle through with incomplete testing.</p></li>
</ul>
<p dir="auto">For this particular diagnostic, I'm not sure about either of those points:</p>
<ul>
<li><p dir="auto">The code pattern is definitely not always harmful. Most importantly, the<br>
behavior is only confusing if the method is actually overridden in a subclass.</p></li>
<li><p dir="auto">There's probably a lot of code which does this innocuously. The usual<br>
admonition that running code is usually correct is stronger than usual here<br>
because most code in a constructor or destructor will actually be run on the<br>
common path.</p></li>
<li><p dir="auto">Some programmers will reasonably object that they know the semantics here.</p></li>
<li><p dir="auto">The workaround can be a bit verbose.</p></li>
<li><p dir="auto">The best workaround is to name the current class, but programmers might find<br>
that weird if it's not where the method is actually defined. If they instead<br>
try to name the defining class, the workaround is making the code more brittle.</p></li>
</ul>
<p dir="auto">So I'm not sure if this is a good warning to add. I agree that it may be better<br>
suited for the static analyzer, which can try to ameliorate at least some of<br>
these concerns.</p>
<p dir="auto"><strong>That said</strong>, I'm concerned about the growing pressure to not add new<br>
warnings. That's not an intended goal of the pressure, but it is the basic<br>
outcome. There is a tightly-woven web of different factors that have the<br>
effect of discouraging the addition of any new diagnostics:</p>
<ul>
<li><p dir="auto">We discourage adding new default-off warnings on the theories that<br>
(1) warnings should have maximal impact and (2) we don't want unused<br>
complexity in the compiler.</p></li>
<li><p dir="auto">New default-on warnings are immediately exercised on a massive amount of<br>
code and will be reverted if they cause any problems for existing code because<br>
we try to keep the codebase as close as possible to an acceptable release<br>
state at all times.</p></li>
<li><p dir="auto">Project maintainers quite reasonably feel that (1) they did not ask for this<br>
new warning and (2) they don't really want to deal with it right now but (3)<br>
it's being forced on them anyway and (4) they are rather upset about that.</p></li>
</ul>
<p dir="auto">The basic problem here is that we have no way for projects to evolve with<br>
default-on warnings, and so we necessarily conflate two very different ideas:</p>
<ul>
<li><p dir="auto">We want the warning to eventually be useful to as many people as possible.</p></li>
<li><p dir="auto">We want the warning to be useful to everyone right now.</p></li>
</ul>
<p dir="auto">But the first doesn't require the second. It would make a lot more sense<br>
if we curated an ever-expanding but <em>versioned</em> set of default-on warnings<br>
and allowed users to ratchet their version forward at their own pace.<br>
More precisely:</p>
<ol>
<li value="1"><p dir="auto">We would add a flag to control the current default diagnostic settings,<br>
something like <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">-Wversion=clang-7</code> to say to use Clang 7's default warning<br>
behavior.</p></li>
<li value="2"><p dir="auto">The first version of that would turn on a whole bunch of warnings that we've<br>
always thought ought to be on by default but aren't because of GCC compatibility.</p></li>
<li value="3"><p dir="auto">In the absence of an explicit <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">-Wversion</code> flag, the compiler would continue<br>
using the supposedly-GCC-compatible defaults.</p></li>
<li value="4"><p dir="auto">It would be understood that <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">-Wversion=X+1</code> will generally be defined as<br>
<code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">-Wversion=X -Wfoo -Wbar -Wbaz</code>. (Not literally in terms of the set-algebra<br>
of warnings flags, just in terms of each version adding new warnings.)</p></li>
<li value="5"><p dir="auto">Project maintainers can roll forward their per-compiler warning versions<br>
when they're ready to deal with the consequences.</p></li>
<li value="6"><p dir="auto">The compiler would not complain if it's passed a future version; it<br>
would just use its most recent rules. This allows projects to easily<br>
roll their warnings version forward while still working on older compilers.<br>
We would clearly document that projects which intentionally post-date<br>
their warnings version do so at their own risk.</p></li>
<li value="7"><p dir="auto">Platform maintainers and distributors like Google and Apple (and other<br>
motivated projects like e.g. Mozilla) would periodically qualify the current<br>
<code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">-Wversion</code> on their codebases in order to gather feedback on the current<br>
set of newly-added warnings.</p></li>
<li value="8"><p dir="auto">Tools like Xcode can make their own decisions about rolling forward warning<br>
versions. For example, a new project might be configured to use the latest<br>
warning settings but then stay with that version until manually updated.</p></li>
</ol>
<p dir="auto">This flag isn't meant to be a guarantee that we'll emit exactly the same<br>
diagnostics as the target version. For one, of course, we might change<br>
diagnostic text or emit the diagnostic somewhere else or add a note or whatever.<br>
More importantly, I think we'd still reserve the right to add new warnings<br>
that are unconditionally on by default if we think they're important or<br>
unproblematic enough.</p>
<p dir="auto">This wouldn't be a blank check to add new default-on warnings: the two basic<br>
considerations above would still apply. It's just that the second<br>
consideration would be far more focused:</p>
<ul>
<li><p dir="auto">It wouldn't need to account for the impact on legacy projects that might<br>
lack an active maintainer. Someone who wants to audit the new warnings on<br>
that project can easily do so, but otherwise the build is unaffected.</p></li>
<li><p dir="auto">It wouldn't need to account for the inter-project politics of compilers<br>
forcing unwanted new warnings on other programmers. A project maintainer<br>
who intentionally rolls their version forward but doesn't like one of the<br>
new warnings can easily deal with that problem themselves.</p></li>
<li><p dir="auto">Instead, it would just evaluate the impact from the perspective of a project<br>
maintainer who has voluntarily requested a larger set of warnings. Again,<br>
that wouldn't be a blank check: the warning still needs to be worthwhile.<br>
But it's necessarily a less fraught prospect.</p></li>
</ul>
<p dir="auto">There would also be a very clear path for warnings under active development:<br>
our expectation would be that most new warnings would eventually be added to<br>
the set of default-on warnings. In my opinion, that leaves room for warnings<br>
to remain default-off as long as we expect that they will eventually become<br>
acceptable to include.</p>
<p dir="auto">John.</p>
</div>
</div>
</body>
</html>