<div dir="ltr"><div>Chromium shouldn't be a factor for determining is a warning is on or off by default. We can easily turn warnings off, and I had done so in <a href="http://crrev.com/http://crrev.com/701604">http://crrev.com/http://crrev.com/701604</a> (as long as warnings have a dedicated flag -- but that's needed for every large codebase adopting a new clang). So that's not super relevant -- the bots would've cycled green anyways.<br></div><div><br></div><div>We should probably write it down, but clang's warning philosophy is that warnings should be very high-signal. The usual way to prove this is to build some large open-source codebase with the warning and counting true and false positives.</div><div><br></div><div>Another thing we sometimes say that a warning should be so good that it should be possible to turn it on by default, and if a warning doesn't meet that bar, then maybe we shouldn't have it. (There are exceptions.)<br><br>ps: To be clear, I appreciate all your work to add warnings a lot! It's very useful. It's also possible that this warning is useful, but it's not immediately clear, so there should be some data to back it up.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 1, 2019 at 1:59 PM Dávid Bolvanský <<a href="mailto:david.bolvansky@gmail.com">david.bolvansky@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Sorry, answered on phone, missed it. I looked at your buildbots for<br>
chromium and yeah, so many warnings. I will make it off by default.<br>
<br>
ut 1. 10. 2019 o 19:58 Dávid Bolvanský <<a href="mailto:david.bolvansky@gmail.com" target="_blank">david.bolvansky@gmail.com</a>> napísal(a):<br>
><br>
> (Not sure if you intentionally didn't reply-all.)<br>
><br>
> Sorry, answered on phone, missed it. I looked at your buildbots for<br>
> chromium and yeah, so many warnings. I will make it off by default.<br>
><br>
> ut 1. 10. 2019 o 19:17 Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> napísal(a):<br>
><br>
> ><br>
> > (Not sure if you intentionally didn't reply-all.)<br>
> ><br>
> > We should probably write it down, but clang's warning philosophy is that warnings should be very high-signal. The usual way to prove this is to build some large open-source codebase with the warning and counting true and false positives.<br>
> ><br>
> > Just "gcc has it" isn't sufficient motivation – gcc has lots of not so good warnings :)<br>
> ><br>
> > ps: To be clear, I appreciate all your work to add warnings a lot! It's very useful. It's also possible that this warning is useful, but it's not immediately clear, so there should be some data to back it up.<br>
> ><br>
> > On Tue, Oct 1, 2019 at 12:12 PM Dávid Bolvanský <<a href="mailto:david.bolvansky@gmail.com" target="_blank">david.bolvansky@gmail.com</a>> wrote:<br>
> >><br>
> >> I hit this bug myself, GCC warned me with -Wenum-compare, Clang was silent. Clang “supports” this flag, but failed to catch it. Either Clang should not pretend that it supports this flag (and all related functionality) or should support it fully. Basically, new warnings will show up only for Clang-only environments.<br>
> >><br>
> >> I think the own subgroup is a good compromise for now. We can still make it off by default before next release if we decide so.<br>
> >><br>
> >> Dňa 1. 10. 2019 o 17:56 užívateľ Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> napísal:<br>
> >><br>
> >> <br>
> >> Thanks!<br>
> >><br>
> >> Any thoughts on the true positive rate for this warning?<br>
> >><br>
> >> On Tue, Oct 1, 2019 at 11:43 AM Dávid Bolvanský <<a href="mailto:david.bolvansky@gmail.com" target="_blank">david.bolvansky@gmail.com</a>> wrote:<br>
> >>><br>
> >>> Yeah, I moved this warning into own subgroup in rL373345. Thanks.<br>
> >>><br>
> >>> ut 1. 10. 2019 o 16:24 Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> napísal(a):<br>
> >>> ><br>
> >>> > Can we move this behind a Wenum-compare subgroup, say Wenum-compare-type? Our codebase is (well, was) Wenum-compare clean, and this new warnings fires pretty often and from a first quick glance the warning looks pretty low-signal anyways (*). Maybe the subgroup shouldn't even be on by default -- do you have any data on true / false positive rate of this?<br>
> >>> ><br>
> >>> > (But for starters, just having a way to turn this off is enough.)<br>
> >>> ><br>
> >>> > For example, we have a windows common control that's either a PGRP_DOWN or a PGRP_UP page control and depending on which you store the control state in the same int, then stuff like `return extra.inner_spin.spin_up ? UPS_DISABLED : DNS_DISABLED;`.<br>
> >>> ><br>
> >>> > On Mon, Sep 30, 2019 at 3:53 PM David Bolvansky via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
> >>> >><br>
> >>> >> Author: xbolva00<br>
> >>> >> Date: Mon Sep 30 12:55:50 2019<br>
> >>> >> New Revision: 373252<br>
> >>> >><br>
> >>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=373252&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=373252&view=rev</a><br>
> >>> >> Log:<br>
> >>> >> [Diagnostics] Warn if enumeration type mismatch in conditional expression<br>
> >>> >><br>
> >>> >> Summary:<br>
> >>> >> - Useful warning<br>
> >>> >> - GCC compatibility (GCC warns in C++ mode)<br>
> >>> >><br>
> >>> >> Reviewers: rsmith, aaron.ballman<br>
> >>> >><br>
> >>> >> Reviewed By: aaron.ballman<br>
> >>> >><br>
> >>> >> Subscribers: cfe-commits<br>
> >>> >><br>
> >>> >> Tags: #clang<br>
> >>> >><br>
> >>> >> Differential Revision: <a href="https://reviews.llvm.org/D67919" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67919</a><br>
> >>> >><br>
> >>> >> Added:<br>
> >>> >> cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c<br>
> >>> >> Modified:<br>
> >>> >> cfe/trunk/lib/Sema/SemaChecking.cpp<br>
> >>> >><br>
> >>> >> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
> >>> >> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=373252&r1=373251&r2=373252&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=373252&r1=373251&r2=373252&view=diff</a><br>
> >>> >> ==============================================================================<br>
> >>> >> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
> >>> >> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Sep 30 12:55:50 2019<br>
> >>> >> @@ -11308,6 +11308,32 @@ static const IntegerLiteral *getIntegerL<br>
> >>> >> return IL;<br>
> >>> >> }<br>
> >>> >><br>
> >>> >> +static void CheckConditionalWithEnumTypes(Sema &S, SourceLocation Loc,<br>
> >>> >> + Expr *LHS, Expr *RHS) {<br>
> >>> >> + QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();<br>
> >>> >> + QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();<br>
> >>> >> +<br>
> >>> >> + const auto *LHSEnumType = LHSStrippedType->getAs<EnumType>();<br>
> >>> >> + if (!LHSEnumType)<br>
> >>> >> + return;<br>
> >>> >> + const auto *RHSEnumType = RHSStrippedType->getAs<EnumType>();<br>
> >>> >> + if (!RHSEnumType)<br>
> >>> >> + return;<br>
> >>> >> +<br>
> >>> >> + // Ignore anonymous enums.<br>
> >>> >> + if (!LHSEnumType->getDecl()->hasNameForLinkage())<br>
> >>> >> + return;<br>
> >>> >> + if (!RHSEnumType->getDecl()->hasNameForLinkage())<br>
> >>> >> + return;<br>
> >>> >> +<br>
> >>> >> + if (S.Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType))<br>
> >>> >> + return;<br>
> >>> >> +<br>
> >>> >> + S.Diag(Loc, diag::warn_conditional_mixed_enum_types)<br>
> >>> >> + << LHSStrippedType << RHSStrippedType << LHS->getSourceRange()<br>
> >>> >> + << RHS->getSourceRange();<br>
> >>> >> +}<br>
> >>> >> +<br>
> >>> >> static void DiagnoseIntInBoolContext(Sema &S, Expr *E) {<br>
> >>> >> E = E->IgnoreParenImpCasts();<br>
> >>> >> SourceLocation ExprLoc = E->getExprLoc();<br>
> >>> >> @@ -11799,6 +11825,8 @@ static void CheckConditionalOperator(Sem<br>
> >>> >> bool Suspicious = false;<br>
> >>> >> CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious);<br>
> >>> >> CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);<br>
> >>> >> + CheckConditionalWithEnumTypes(S, E->getBeginLoc(), E->getTrueExpr(),<br>
> >>> >> + E->getFalseExpr());<br>
> >>> >><br>
> >>> >> if (T->isBooleanType())<br>
> >>> >> DiagnoseIntInBoolContext(S, E);<br>
> >>> >><br>
> >>> >> Added: cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c<br>
> >>> >> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c?rev=373252&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c?rev=373252&view=auto</a><br>
> >>> >> ==============================================================================<br>
> >>> >> --- cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c (added)<br>
> >>> >> +++ cfe/trunk/test/Sema/warn-conditional-emum-types-mismatch.c Mon Sep 30 12:55:50 2019<br>
> >>> >> @@ -0,0 +1,39 @@<br>
> >>> >> +// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare %s<br>
> >>> >> +// RUN: %clang_cc1 -x c -fsyntax-only -verify %s<br>
> >>> >> +// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare %s<br>
> >>> >> +// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s<br>
> >>> >> +<br>
> >>> >> +enum ro { A = 0x10 };<br>
> >>> >> +enum rw { B = 0xFF };<br>
> >>> >> +enum { C = 0x1A};<br>
> >>> >> +<br>
> >>> >> +enum {<br>
> >>> >> + STATUS_SUCCESS,<br>
> >>> >> + STATUS_FAILURE,<br>
> >>> >> + MAX_BASE_STATUS_CODE<br>
> >>> >> +};<br>
> >>> >> +<br>
> >>> >> +enum ExtendedStatusCodes {<br>
> >>> >> + STATUS_SOMETHING_INTERESTING = MAX_BASE_STATUS_CODE + 1000,<br>
> >>> >> +};<br>
> >>> >> +<br>
> >>> >> +<br>
> >>> >> +int get_flag(int cond) {<br>
> >>> >> + return cond ? A : B;<br>
> >>> >> + #ifdef __cplusplus<br>
> >>> >> + // expected-warning@-2 {{enumeration type mismatch in conditional expression ('ro' and 'rw')}}<br>
> >>> >> + #else<br>
> >>> >> + // expected-no-diagnostics<br>
> >>> >> + #endif<br>
> >>> >> +}<br>
> >>> >> +<br>
> >>> >> +// In the following cases we purposefully differ from GCC and dont warn because<br>
> >>> >> +// this code pattern is quite sensitive and we dont want to produce so many false positives.<br>
> >>> >> +<br>
> >>> >> +int get_flag_anon_enum(int cond) {<br>
> >>> >> + return cond ? A : C;<br>
> >>> >> +}<br>
> >>> >> +<br>
> >>> >> +int foo(int c) {<br>
> >>> >> + return c ? STATUS_SOMETHING_INTERESTING : STATUS_SUCCESS;<br>
> >>> >> +}<br>
> >>> >><br>
> >>> >><br>
> >>> >> _______________________________________________<br>
> >>> >> cfe-commits mailing list<br>
> >>> >> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> >>> >> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>