<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Thanks for the detailed answer.<br></div><div dir="ltr"><br></div><div dir="ltr">> The code pattern is definitely not always harmful. Most importantly, the<br></div><div dir="ltr">> behavior is only confusing if the method is actually overridden in a subclass.</div><div><br></div><div>In this regard, note that my patch won't warn if the class is final.</div><div>Otherwise, indeed it might work as expected *now* but won't once reimplemented in a subclass. Qualifying the call make things clearer.</div><div><br></div><div>> 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.</div><div><br></div><div>Yes indeed: I named the *current* class instead of the defining one intentionally in the fixit, because that would make the code behaves like if it wasn't qualify, if one day the method gets reimplemented in the current class.</div><div>If the developer decides qualify the call using the defining class anyway, at least it will be clear what implementation will get called (but agreed this would make the code more brittle).</div><div><br></div><div>I realized I didn't put "DefaultIgnore" on this warning.</div><div>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.</div><div><br></div><div>What do you think?</div><div><br></div><div>Arnaud<br></div><div><br></div><div><br></div><div><br></div><div class="gmail_quote"><div dir="ltr">Le mar. 8 janv. 2019 à 11:59, John McCall via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</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"><u></u>
<div>
<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 rgb(119,119,119);color:rgb(119,119,119);margin:0px 0px 5px;padding-left:5px"><p dir="auto">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:</p>
<blockquote style="border-left:2px solid rgb(153,153,153);color:rgb(153,153,153);margin:0px 0px 5px;padding-left:5px"><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 rgb(153,153,153);color:rgb(153,153,153);margin:0px 0px 5px;padding-left:5px"><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"><b>That said</b>, 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 <i>versioned</i> 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:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 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:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 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:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 0.4em" bgcolor="#F7F7F7">-Wversion=X+1</code> will generally be defined as<br>
<code style="background-color:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 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:rgb(247,247,247);border-radius:3px;margin:0px;padding:0px 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>
</div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div></div></div>