<div dir="ltr"><div>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.</div><div>I was initially convinced by Aaron that adding default-off warnings might not be interesting, since not many people are likely to use it.</div><div>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">https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors</a></div><div>(maybe some Google C++ developers here can comment on how strictly those guidelines are enforced in their projects).<br></div><div>CppCoreGuidelines also discourage this: <a href="https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-ctor-virtual">https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-ctor-virtual</a></div><div><br></div><div>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.</div><div><br></div><div>With this in mind, do you think we can reconsider having such a warning implemented in clang?</div><div><br></div><div>Arnaud<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 14 janv. 2019 à 20:49, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">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 Mon, Jan 14, 2019 at 2:44 PM John McCall <<a href="mailto:jmccall@apple.com" target="_blank">jmccall@apple.com</a>> wrote:<br>
><br>
> On 14 Jan 2019, at 14:40, Aaron Ballman wrote:<br>
> > On Mon, Jan 14, 2019 at 2:31 PM John McCall <<a href="mailto:jmccall@apple.com" target="_blank">jmccall@apple.com</a>> wrote:<br>
> >> On 14 Jan 2019, at 14:23, Aaron Ballman wrote:<br>
> >>> On Mon, Jan 14, 2019 at 1:59 PM John McCall via cfe-dev<br>
> >>> <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
> >>>><br>
> >>>> On 11 Jan 2019, at 11:23, Arnaud Bienner wrote:<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<br>
> >>>>><br>
> >>>>> 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.<br>
> >>>><br>
> >>>> I agree.<br>
> >>>><br>
> >>>>> 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?<br>
> >>>><br>
> >>>> I think it should go in, default-off, and we should implement my<br>
> >>>> proposal<br>
> >>><br>
> >>> 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.<br>
> >><br>
> >> 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.<br>
> ><br>
> > 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)?<br>
><br>
> 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.<br>
<br>
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>
<br>
><br>
> John.<br>
><br>
> ><br>
> > ~Aaron<br>
> ><br>
> >><br>
> >> John.<br>
> >><br>
> >>><br>
> >>> ~Aaron<br>
> >>><br>
> >>>> 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>
> >>>><br>
> >>>>><br>
> >>>>> Arnaud<br>
> >>>>><br>
> >>>>><br>
> >>>>><br>
> >>>>><br>
> >>>>><br>
> >>>>> Le mer. 9 janv. 2019 à 01:14, John McCall <<a href="mailto:jmccall@apple.com" target="_blank">jmccall@apple.com</a>> a<br>
> >>>>> écrit :<br>
> >>>>><br>
> >>>>>> 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 <<a href="mailto:jmccall@apple.com" target="_blank">jmccall@apple.com</a>><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>
> >>>>>> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> 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>
> >>>>>><br>
> >>>><br>
> >>>><br>
> >>>> _______________________________________________<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>