LGTM As we discussed offline, we should check whether the E is present on a pointer-to-member-function mangling (if so, maybe it should be added when mangling the function type, not when mangling the function decl).
On Fri, May 10, 2013 at 2:37 PM, Warren Hunt <[email protected]> wrote: > 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 >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
