[cfe-dev] please review: fix for http://llvm.org/PR4407

[Chris Lattner]

On Oct 14, 2009, at 1:48 PM, Zhanyong Wan (λx.x x) wrote:
> Hi,
>
> I have a patch that fixes the first issue in http://llvm.org/PR4407 :
>
> Clang should warn on a case value that exceeds the range of the type  
> in
> switch.
>  const char ch = 'a';
>  switch(ch) {
>    case 1234:  // expected-warning {{overflow converting case value}}
>      break;
>  }
>
> I uploaded the patch to http://codereview.appspot.com/130078/show.
> Could someone review it?
>
> This is the first time I work on clang, so please let me know if I'm
> following the coding convention correctly.  Thanks,

Hi Zhanyong,

Welcome to the community, thank you for tackling this!

Please attach patches to email instead of linking to codereview.   
Personally I find it a lot easier to review the patch as an attachment.

Some general points:

   const ImplicitCastExpr * const ImplicitCast

We generally don't bother declaring local variables const, something  
like this:
   const ImplicitCastExpr *ImplicitCast

... is preferred, likewise with 'const QualType', ExprBeforePromotion,  
etc.


Your test looks very nice.  Please resend with these changes, thanks!

-Chris