[cfe-dev] please review: fix for http://llvm.org/PR4407
Chris Lattner
clattner at apple.com
Wed Oct 14 16:46:01 PDT 2009
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
More information about the cfe-dev
mailing list