Addressing BNF comments request. -Warren
On Thu, May 9, 2013 at 3:49 PM, Richard Smith <[email protected]> wrote: > On Thu, May 9, 2013 at 11:32 AM, Warren Hunt <[email protected]> wrote: > >> I updated the white space. >> >> Are these are the checks for pointers/references to functions that you >> were looking for? (lines 128-134 of mangle-ms-arg-qualifiers.cpp) >> > > Ah, yes, thanks :) > > >> void foo_p6ahxz(int x()) {} >> // CHECK: "\01?foo_p6ahxz@@YAXP6AHXZ@Z" >> // X64: "\01?foo_p6ahxz@@YAXP6AHXZ@Z" >> >> void foo_a6ahxz(int (&x)()) {} >> // CHECK: "\01?foo_a6ahxz@@YAXA6AHXZ@Z" >> // X64: "\01?foo_a6ahxz@@YAXA6AHXZ@Z" >> >> void foo_q6ahxz(int (&&x)()) {} >> // CHECK: "\01?foo_q6ahxz@@YAX$$Q6AHXZ@Z" >> // X64: "\01?foo_q6ahxz@@YAX$$Q6AHXZ@Z" >> >> >> >> On Wed, May 8, 2013 at 3:39 PM, Richard Smith <[email protected]>wrote: >> >>> @@ -1277,6 +1284,8 @@ void >>> MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) { >>> else >>> Out << 'Q'; >>> } >>> + if (PointersAre64Bit && !MD->isStatic()) >>> + Out << 'E'; >>> } else >>> Out << 'Y'; >>> } >>> >>> Needs more indentation. >>> >>> You have special cases for pointers/references to functions (where no E >>> is added) but I don't see any test cases for that. >>> >>> The BNF comments should be updated in various places to indicate the >>> presence of this specifier. It might also be useful to factor out the "if >>> (PointersAre64Bit) Out << 'E';" code into a function, and make up a name >>> for it to use in the BNF descriptions. >>> >> > I'd still like these comments updated. Other than that, patch looks fine. > > >> On Wed, May 8, 2013 at 1:36 PM, Warren Hunt <[email protected]> wrote: >>> >>>> ping >>>> >>>> >>>> On Fri, May 3, 2013 at 12:07 PM, Warren Hunt <[email protected]> wrote: >>>> >>>>> Sounds good. I added a local that caches the 64-bitness of pointers. >>>>> >>>>> -Warren >>>>> >>>>> >>>>> On Thu, May 2, 2013 at 2:51 PM, Charles Davis <[email protected]>wrote: >>>>> >>>>>> >>>>>> On May 2, 2013, at 3:19 PM, Warren Hunt wrote: >>>>>> >>>>>> > I've put together a patch that fixes all of the issues I found with >>>>>> Microsoft name mangling in 64-bit mode. Mostly it involves adding an 'E' >>>>>> note to all pointers that are 64-bit (including 'this' pointers). This >>>>>> is >>>>>> done in MicrosoftMangle.cpp. I've updated 3 of the mangling test files >>>>>> with X64 tests, which provides some reasonable amount of coverage. >>>>>> > >>>>>> > Please review. >>>>>> > >>>>>> > Thanks, >>>>>> > -Warren >>>>>> > >>>>>> > (Note: This is my first patch.) >>>>>> Welcome to Clang development! >>>>>> >>>>>> > @@ -1277,6 +1278,9 @@ void >>>>>> MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) { >>>>>> > else >>>>>> > Out << 'Q'; >>>>>> > } >>>>>> > + if (getASTContext().getTargetInfo().getPointerWidth(0) == 64 && >>>>>> > + !MD->isStatic()) >>>>>> > + Out << 'E'; >>>>>> > } else >>>>>> > Out << 'Y'; >>>>>> > } >>>>>> The indentation on this looks wrong. (You can't tell from here, but >>>>>> it's definitely wrong in the patch itself.) Also, I think we should >>>>>> handle >>>>>> this where we mangle in the 'this' qualifiers--the 'E' is because the >>>>>> 'this' pointer is Extended (__ptr64 instead of __ptr32). (The relevant >>>>>> line >>>>>> is 1172.) >>>>>> >>>>>> > @@ -1390,6 +1394,8 @@ void >>>>>> MicrosoftCXXNameMangler::mangleDecayedArrayType(const ArrayType *T, >>>>>> > manglePointerQualifiers(T->getElementType().getQualifiers()); >>>>>> > } else { >>>>>> > Out << 'Q'; >>>>>> > + if (getASTContext().getTargetInfo().getPointerWidth(0) == 64) >>>>>> > + Out << 'E'; >>>>>> > } >>>>>> > mangleType(T->getElementType(), SourceRange()); >>>>>> > } >>>>>> Why only here? What does VC do in the global case? >>>>>> >>>>>> There's a lot of code duplication (i.e. checks against the pointer >>>>>> size sprinkled throughout the code). Most of those can be collapsed into >>>>>> mangleQualifiers(). >>>>>> >>>>>> Someone else wrote a patch a while back. You can find it here: >>>>>> >>>>>> http://llvm-reviews.chandlerc.com/D101 >>>>>> >>>>>> I didn't finish reviewing it because real life got in the way. :\. >>>>>> You might find some ideas in there. >>>>>> >>>>>> Chip >>>>>> >>>>>> >>>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>> >> >
x64mangle3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
