[cfe-commits] [PATCH] PR11778 Switch promotions working properly

[Richard Smith]

+def warn_case_value_cannot_be_reached : Warning<
+  "case value '%0' is not reachable given the switch condition type:
'%1'">,
+  InGroup<DiagGroup<"switch">>;

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:

"case value %0 cannot be reached because switch condition is of type %1"

or just

"switch condition of type %1 never has value %0" ?

@@ -722,15 +730,26 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc,
Stmt *Switch,
         Lo = ImpCastExprToType(Lo, CondType, CK_IntegralCast).take();
       }

-      // Convert the value to the same width/sign as the condition had
prior to
-      // integral promotions.
-      //
-      // FIXME: This causes us to reject valid code:
-      //   switch ((char)c) { case 256: case 0: return 0; }
-      // Here we claim there is a duplicated condition value, but there is
not.
-      ConvertIntegerToTypeWarnOnOverflow(LoVal, CondWidth, CondIsSigned,
+      // Convert the value to the same width/sign as the condition.
+      llvm::APSInt LoValTemp(LoVal);
+      if (ConvertIntegerToTypeWarnOnOverflow(LoVal, CondWidth,
+                                         CondIsSigned,
                                          Lo->getLocStart(),
-                                         diag::warn_case_value_overflow);
+                                         diag::warn_case_value_overflow) &&

Please fix the indentation here.

+          CondWidthBeforePromotion < CondWidth &&
+          CondWidthBeforePromotion < LoValTemp.getBitWidth()) {

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.

+          Diag(Lo->getLocStart(), diag::warn_case_value_cannot_be_reached)
+              << LoValTemp.toString(10)
+              << CondTypeBeforePromotion.getAsString();

Don't use getAsString, just pass the QualType directly into the diagnostic
here.

+          continue;

This 'continue' seems to be incorrect.

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.

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.

On Fri, Aug 31, 2012 at 1:55 PM, Ahmed Charles <acharles at outlook.com> wrote:

> Ping. :)
>
>
> ------------------------------
> From: acharles at outlook.com
> To: dblaikie at gmail.com
> Date: Wed, 29 Aug 2012 01:39:33 -0700
>
> CC: cfe-commits at cs.uiuc.edu
> Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working
> properly
>
> I moved the test cases to more reasonable files.
>
> ------------------------------
> From: acharles at outlook.com
> To: dblaikie at gmail.com
> Date: Tue, 28 Aug 2012 15:37:26 -0700
> CC: cfe-commits at cs.uiuc.edu
> Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working
> properly
>
> New patch, with test cases.
>
> > Date: Tue, 28 Aug 2012 13:18:52 -0700
> > Subject: Re: [cfe-commits] [PATCH] PR11778 Switch promotions working
> properly
> > From: dblaikie at gmail.com
> > To: acharles at outlook.com
> > CC: cfe-commits at cs.uiuc.edu
> >
> > Could you include the test cases in your patch?
> >
> > On Tue, Aug 28, 2012 at 1:11 PM, Ahmed Charles <acharles at outlook.com>
> wrote:
> > > This makes switch statements like the following compile:
> > >
> > > int f1(char c) { switch(c) { case 256: case 0: return 1; } return 0; }
> > > int f2(unsigned char c) { switch(c) { case 256: case 0: return 1; }
> return
> > > 0; }
> > > int f3(char c) { switch(c) { case 255 ... 256: case 0: return 1; }
> return 0;
> > > }
> > > int f3(unsigned char c) { switch(c) { case 255 ... 256: case 0: return
> 1; }
> > > return 0; }
> > >
> > > The condition in the switch statement undergoes integer promotions,
> > > therefore the above should compile, but doesn't. This also adds a
> warning
> > > which tells the user when a case can't be reached.
> > >
> > > _______________________________________________
> > > cfe-commits mailing list
> > > cfe-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > >
>
> _______________________________________________ cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________ cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120905/de18a09e/attachment.html>