- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 656
fix Issue 18053 - Use stdint.h mangling for int64_t/uint64_t when man… #7416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| Thanks for your pull request, @WalterBright! Bugzilla references
 | 
a6ca0bb    to
    ceaf4e6      
    Compare
  
    | Note how much nicer the test cases get :-) | 
ceaf4e6    to
    ba1a6e9      
    Compare
  
    | @ibuclaw @klickverbot please check effect on gdc/ldc | 
        
          
                src/ddmd/cppmangle.d
              
                Outdated
          
        
      | case Tint64: | ||
| c = (Target.c_longsize == 8 ? 'l' : 'x'); | ||
| // Mangle like target's stdint.h int64_t | ||
| c = (Target.c_longsize == 4 || global.params.isOSX ? 'x' : 'l'); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be turned into a target hook. The frontend should not be asking questions such as "Am I targeting osx?". I think we already have a mangle type hook already present, the logic can be put there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 459 to 465 in 5494dba
| * For a vendor-specific type, return a string containing the C++ mangling. | |
| * In all other cases, return null. | |
| */ | |
| extern (C++) static const(char)* cppTypeMangle(Type t) | |
| { | |
| return null; | |
| } | 
Comment would need to be updated to reflect that if the target requires mangling in a specific way, then handle it here.
| I'm not sure which C++ compiler you're looking at but these are the results I got with Xcode 9.0 containing Clang  64bit:
 32bit:
 As can be seen in these tables, D uses the wrong mangling for the  I've used the following C++ code: #include <stdint.h>
void a(long){}
void b(long long){}
void c(int64_t){}
void d(unsigned long){}
void e(unsigned long long){}
void f(uint64_t){}And the following D code: extern (C++):
import core.stdc.config;
import core.stdc.stdint;
void a(c_long){}
void b(long){}
void c(int64_t){}
void d(c_ulong){}
void e(ulong){}
void f(uint64_t){} | 
| Hmm, seems like  | 
| BTW, why this complicated logic with a struct with a specific name that matches the mangling of C++? Couldn't we make the following to work? pragma(mangle, "m") alias c_ulong = ulong;That would also avoid the need to explicitly instantiate the struct, since D doesn't have support for implicit conversions in this case. The above solution is more generic and nothing in the compiler needs to be hard coded like it is now. | 
| 
 There is no right mangling for the long type, because the C long may be 32 bits or 64 bits. 
 I'd welcome a better solution for that. But not in this PR. | 
ba1a6e9    to
    828979f      
    Compare
  
    Implemented Target.int64Mangle and Target.uint64Mangle
828979f    to
    e734c19      
    Compare
  
    | 
 I have something that is a work in progress. | 
| @WalterBright Can you please rebase? | 
…gling D long/ulong
e734c19    to
    517253e      
    Compare
  
    | This broke the OSX build on Travis, which shows the problems @jacob-carlborg pointed out. Should we revert, s.t. the error can be properly fixed/worked out? | 
…gling D long/ulong
I'm tired of expending many hours dealing with this issue again and again. Fortunately, it only changes the mangling on OSX 64.