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

[Zhanyong Wan (λx.x x)]

2009/10/14 Chris Lattner <clattner at apple.com>:
> 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!

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. :-)

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. :-)

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

Will send to cfe-commits.  Thanks!

>
> -Chris



-- 
Zhanyong