Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

[Trivial] OS X: Make core.stdc.stdint independent of D long C++-mangling#2191

Merged
dlang-bot merged 1 commit intodlang:masterfrom
kinke:osx64_long
May 24, 2018
Merged

[Trivial] OS X: Make core.stdc.stdint independent of D long C++-mangling#2191
dlang-bot merged 1 commit intodlang:masterfrom
kinke:osx64_long

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented May 23, 2018

C++ int64_t is defined as long long on both 32/64-bit OS X, so use the appropriate alias defined in core.stdc.config.

@kinke kinke requested review from ibuclaw and schveiguy as code owners May 23, 2018 20:31
@dlang-bot dlang-bot added the Trivial typos, formatting, comments label May 23, 2018
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2191"

@schveiguy
Copy link
Member

It looks reasonable. Is there a way to know that it's correct? There are no unittests for this kind of stuff it seems.

@kinke
Copy link
Contributor Author

kinke commented May 23, 2018

There are DMD mangling interop tests (or at least will be); I'm about to create a DMD PR reverting to the previous mangling for long on 64-bit OS X; this is a prerequisite to get it working.

@schveiguy
Copy link
Member

I have a mac, is there something I can compile to ensure this is correct?

@kinke
Copy link
Contributor Author

kinke commented May 23, 2018

For testing, you could compile

import core.stdc.stdint;
extern(C++) void fooCpp(int64_t, uint64_t);
void main() { foo(123, 456); }

(with DMD master) and check the function mangle. It should be __Z6fooCpplm according to https://godbolt.org/g/iaqA2y - edit: that seems to be wrong (wrong headers for cross-compilation?), it should be __Z6fooCppxy AFAIK, so maybe try compiling C++ and linking then to be safe.

@schveiguy
Copy link
Member

schveiguy commented May 23, 2018

I'm confused -- I wrote the following C++ file:

#include <stdint.h>

void fooCpp(int64_t, uint64_t)
{
}

And it mangles as __Z6fooCppxy

If I write your code (changing foo to fooCpp of course), it links fine against the C++ object on the 2.080 compiler (without your changes).

I don't know what the plan is here, but it seems like we shouldn't be ultimately changing the mangling at all, no?

@kinke
Copy link
Contributor Author

kinke commented May 23, 2018

So it's working (and this stuff is in LDC 1.10). ;) - This tiny PR here doesn't change the mangling, it just prepares for a D long mangled as C++ long again without breaking core.stdc.stdint.int64_t and friends. It's just a more robust way of defining them as C++ long long on Mac.

@dlang-bot dlang-bot merged commit 54ab96e into dlang:master May 24, 2018
@schveiguy
Copy link
Member

Sorry, wish I could have helped better, but my understanding of the OSX mangling issues is not good...

@kinke kinke deleted the osx64_long branch May 24, 2018 09:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Trivial typos, formatting, comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants