Hi Jordan,
Thanks for reviewing. 1. Sema.h is removed. 2. I glad to find that we don't need the workaround for the exception. Someone fixed something in clang. So we don't need to relax the assertion. 3. Fixed. To be consistent with "IsThumb", I name the variable "IsAAPCSS". But I can't completely decide it as constructor because setABI() will change the ABI. For consistency, the variable name. Please help to review the attached patch. Thanks, Weiming Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation From: Jordan Rose [mailto:[email protected]] Sent: Thursday, October 04, 2012 6:19 PM To: Logan Chien Cc: Weiming Zhao; [email protected] Subject: Re: [cfe-commits] Bug 11709 Fix: va_list on ARM is not following AAPCS 7.1.4 A couple comments (sorry for the long long delay): - AST can't depend on Sema. Is there a reason for the include of Sema.h in ASTContext.cpp? - It would be a good idea to leave a comment in ExprClassification.cpp about why __va_list needs an exception. - For ARMTargetInfo::getBuiltinVaListKind, maybe you can just decide this at construction time and stick it in a local variable? 'unsigned usesAAPCS : 1'? Thanks for working on this, Weiming, Logan. Jordan On Oct 2, 2012, at 22:34 , Logan Chien <[email protected]> wrote: LGTM. However, I think we can enhance the test cases a little. 1. Keep the void* test in builtins-arm.c for apcs-gnu ABI. 2. Add the unit test for va_list name mangling. Sincerely, Logan ps. The attached patch is the revised patch with the updated test cases. On Thu, Sep 20, 2012 at 6:13 AM, Weiming Zhao <[email protected]> wrote: Ping... Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation <mangle-valist.cpp><builtins-arm.c><0001-Bug-11709-Change-the-definition-of- va_list-to-meet-A.patch>
0004-Fix-PR-11709-Change-the-definition-of-va_list-to-mee.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
