<div dir="ltr">Thanks!<div><br>Any thoughts on the true positive rate for this warning?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 1, 2019 at 11:43 AM 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">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>