<div><div>+def warn_case_value_cannot_be_reached : Warning<</div><div>+ "case value '%0' is not reachable given the switch condition type: '%1'">,</div><div>+ InGroup<DiagGroup<"switch">>;</div>
</div><div><br></div><div>Please reword this to include the type in the prose. Also, do not put ''s around a placeholder for a type, nor for an integer value. Perhaps:</div><div><br></div><div>"case value %0 cannot be reached because switch condition is of type %1"</div>
<div><br></div><div>or just</div><div><br></div><div>"switch condition of type %1 never has value %0" ?</div><div><br></div><div>@@ -722,15 +730,26 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,</div>
<div> Lo = ImpCastExprToType(Lo, CondType, CK_IntegralCast).take();</div><div> }</div><div> </div><div>- // Convert the value to the same width/sign as the condition had prior to</div><div>- // integral promotions.</div>
<div>- //</div><div>- // FIXME: This causes us to reject valid code:</div><div>- // switch ((char)c) { case 256: case 0: return 0; }</div><div>- // Here we claim there is a duplicated condition value, but there is not.</div>
<div>- ConvertIntegerToTypeWarnOnOverflow(LoVal, CondWidth, CondIsSigned,</div><div>+ // Convert the value to the same width/sign as the condition.</div><div>+ llvm::APSInt LoValTemp(LoVal);</div><div>+ if (ConvertIntegerToTypeWarnOnOverflow(LoVal, CondWidth,</div>
<div>+ CondIsSigned,</div><div> Lo->getLocStart(),</div><div>- diag::warn_case_value_overflow);</div>
<div>+ diag::warn_case_value_overflow) &&</div><div><br></div><div>Please fix the indentation here.</div><div><br></div><div><div>+ CondWidthBeforePromotion < CondWidth &&</div>
<div>+ CondWidthBeforePromotion < LoValTemp.getBitWidth()) {</div></div><div><br></div><div>I don't think the second check here is correct. Suppose LoValTemp is a negative signed char and CondTypeBeforePromotion is unsigned char or unsigned short. Then CondType is int, and CondWidthBeforePromotion >= LoValTemp.getBitWidth(), so we'll skip the check, even though we should issue a diagnostic here.</div>
<div><br></div><div><div>+ Diag(Lo->getLocStart(), diag::warn_case_value_cannot_be_reached)</div><div>+ << LoValTemp.toString(10)</div><div>+ << CondTypeBeforePromotion.getAsString();</div>
<div><br></div><div>Don't use getAsString, just pass the QualType directly into the diagnostic here.</div><div><br></div><div>+ continue;</div></div><div><br></div><div>This 'continue' seems to be incorrect.</div>
<div><br></div><div>Also, since you perform the same check and issue the same diagnostic in two different places, please factor that code out into a separate function.</div><div><br></div><div>Finally, for a GNU-style case range, I think the ideal behavior would be to warn only if no part of the case range is reachable, rather than warning if either end is unreachable, but I'm happy for this to go in without that tweak.</div>
<br><div class="gmail_quote">On Fri, Aug 31, 2012 at 1:55 PM, Ahmed Charles <span dir="ltr"><<a href="mailto:acharles@outlook.com" target="_blank">acharles@outlook.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div dir="ltr"><font style="font-size:10pt" color="#000000" face="Verdana" size="2">Ping. :)<br></font><br><br><div><div class="im"><div></div><hr>From: <a href="mailto:acharles@outlook.com" target="_blank">acharles@outlook.com</a><br>
To: <a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a><br></div>Date: Wed, 29 Aug 2012 01:39:33 -0700<div><div class="h5"><br>CC: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working properly<br><br>
<div dir="ltr">I moved the test cases to more reasonable files.<br><br><div><div></div><hr>From: <a href="mailto:acharles@outlook.com" target="_blank">acharles@outlook.com</a><br>To: <a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a><br>
Date: Tue, 28 Aug 2012 15:37:26 -0700<br>CC: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working properly<br><br>
<div dir="ltr">New patch, with test cases.<br><br><div><div></div>> Date: Tue, 28 Aug 2012 13:18:52 -0700<br>> Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working properly<br>> From: <a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a><br>
> To: <a href="mailto:acharles@outlook.com" target="_blank">acharles@outlook.com</a><br>> CC: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>> <br>> Could you include the test cases in your patch?<br>
> <br>> On Tue, Aug 28, 2012 at 1:11 PM, Ahmed Charles <<a href="mailto:acharles@outlook.com" target="_blank">acharles@outlook.com</a>> wrote:<br>> > This makes switch statements like the following compile:<br>
> ><br>> > int f1(char c) { switch(c) { case 256: case 0: return 1; } return 0; }<br>> > int f2(unsigned char c) { switch(c) { case 256: case 0: return 1; } return<br>> > 0; }<br>> > int f3(char c) { switch(c) { case 255 ... 256: case 0: return 1; } return 0;<br>
> > }<br>> > int f3(unsigned char c) { switch(c) { case 255 ... 256: case 0: return 1; }<br>> > return 0; }<br>> ><br>> > The condition in the switch statement undergoes integer promotions,<br>
> > therefore the above should compile, but doesn't. This also adds a warning<br>> > which tells the user when a case can't be reached.<br>> ><br>> > _______________________________________________<br>
> > cfe-commits mailing list<br>> > <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> ><br></div> </div>
<br>_______________________________________________
cfe-commits mailing list
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div> </div>
<br>_______________________________________________
cfe-commits mailing list
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></div></div> </div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br>