please review: fix for http://llvm.org/PR4407

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,

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.

The URL doesn't include the period in the end:
http://codereview.appspot.com/130078/show

Thanks,

Resending to cfe-commits. You can tell I'm doing this for the first time. :slight_smile:

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

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!

Thanks!

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

Will do. I found out that patches are supposed to be sent to
cfe-commits@, so I'll follow up on that mailing list instead.

BTW, the codereview tool lets you see more context as needed, and
makes it very easy to diff two versions of the patch. I use it in my
open-source project (googletest) extensively and have found it really
helpful. Therefore I'm hoping the clang team can consider using it
(maybe optionally) in the future.

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.

OK. I figured out the convention from the surrounding code. Was
trying to see if the rule could be bent. :slight_smile:

My experience is that declaring local variables as const helps the
readability and catches programmer errors. Perhaps we can re-consider
this guideline one day, too. :slight_smile:

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

Will send to cfe-commits. Thanks!