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