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

Conversation

@thewilsonator
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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#2349"

@thewilsonator
Copy link
Contributor Author

Hmm

src/core/stdc/wchar_.d(86): Error: cannot implicitly convert expression `65535` of type `int` to `__c_wchar_t`
src/core/stdc/config.d(159): Error: enum `core.stdc.config.__c_wchar_t` is forward referenced when looking for `min`

I thought it was supposed to build with the latest dmd master, that hack was extended to __c_wchar_t.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 12, 2018

src\rt\dmain2.d(115): Error: function `core.sys.windows.winbase.LoadLibraryW(const(wchar)*)` is not callable using argument types `(const(__c_wchar_t*))`
src\rt\dmain2.d(115):        cannot pass argument `name` of type `const(__c_wchar_t*)` to parameter `const(wchar)*`

These function signatures need to be updated to use wchar_t instead I guess.

winnt.d: alias const(wchar)* LPCWCH, PCWCH, LPCWSTR, PCWSTR;

@thewilsonator
Copy link
Contributor Author

thewilsonator commented Nov 12, 2018

Hmm, looks like CircleCI builds with 2.079.1, which obviously is not going to work as this requires DMD master.

@thewilsonator
Copy link
Contributor Author

Looks like it grabs a dmd branch by the same name if it can.

@thewilsonator
Copy link
Contributor Author

thewilsonator commented Nov 13, 2018

... or not.

@thewilsonator
Copy link
Contributor Author

@ibuclaw should be ready to go.

@thewilsonator thewilsonator force-pushed the thewilsonator-patch-1 branch 7 times, most recently from 7168034 to 14c142f Compare November 13, 2018 05:58
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows bindings looks like horrible (even without this patch), ok.

@thewilsonator
Copy link
Contributor Author

The only thing I'm worried about is implicit conversions from string to immutable(wchar_t)[] failing to happen breaking code. This requires people to actually use ""w and ""w.ptr properly.

@rainers
Copy link
Member

rainers commented Nov 13, 2018

The only thing I'm worried about is implicit conversions from string to immutable(wchar_t)[] failing to happen breaking code. This requires people to actually use ""w and ""w.ptr properly.

We had this with c_long when it was a struct, too. I suspect this will cause a lot of breakage. I haven't tried but ""w.ptr is probably not good enough, you still have to cast it with cast(immutable(wchar_t)*)""w.ptr. Or do they convert implicitly?

I'm not sure the windows definitions have to change to begin with, these are C APIs, not C++.

@thewilsonator
Copy link
Contributor Author

Removed most of the windows diff, yeah pointer conversions don't work yet, need to fix that in DMD.

@thewilsonator
Copy link
Contributor Author

this needs
dlang/dmd#8958

@WalterBright
Copy link
Member

Here's the trouble:

src\rt\dmain2.d(115): Error: function `core.sys.windows.winbase.LoadLibraryW(const(wchar)*)` is not callable using argument types `(const(__c_wchar_t*))`
src\rt\dmain2.d(115):        cannot pass argument `name` of type `const(__c_wchar_t*)` to parameter `const(wchar)*`

This is brought on by the following line in core.sys.windows.winnt:

alias wchar WCHAR;

The interesting thing is that the Windows API functions do not use name mangling. So the name mangling isn't an issue. The problem is really in src\rt\dmain2.d, i.e. declare rt_loadLibraryW(const wchar_t* name) instead as rt_loadLibraryW(const WCHAR* name) instead, and all the other uses of wchar_t in the file.

This should be future proof, as it will follow whatever Microsoft does with WCHAR.

@WalterBright
Copy link
Member

#2389

@WalterBright
Copy link
Member

Now that #2389 has merged, this should be ready to rock.

@WalterBright
Copy link
Member

src\rt\dmain2.d(386): Error: function `core.stdc.wchar_.wcslen(scope const(__c_wchar_t*) s)` is not callable using argument types `(const(wchar*))`
src\rt\dmain2.d(386):        cannot pass argument `wCommandLine` of type `const(wchar*)` to parameter `scope const(__c_wchar_t*) s`
src\rt\dmain2.d(403): Error: function `core.stdc.wchar_.wcslen(scope const(__c_wchar_t*) s)` is not callable using argument types `(wchar*)`
src\rt\dmain2.d(403):        cannot pass argument `wargs[cast(ulong)i]` of type `wchar*` to parameter `scope const(__c_wchar_t*) s`

A cast is needed to call wcslen().

@thewilsonator
Copy link
Contributor Author

That is exactly what dlang/dmd#8958 was supposed to fix.

@thewilsonator
Copy link
Contributor Author

I mean, I can add it, but it doesn't fix the underlying issue and the same thing will affect users as well.

@WalterBright
Copy link
Member

An alternative is to change the declaration of WCHAR from wchar to wchar_t. This would match what Microsoft is doing.

Argh. There are no good solutions.

@thewilsonator
Copy link
Contributor Author

https://docs.microsoft.com/en-us/windows/desktop/extensiblestorageengine/gg269344(v%3Dexchg.10)

#if !defined(_NATIVE_WCHAR_T_DEFINED)
typedef unsigned short WCHAR;
#else
typedef wchar_t WCHAR;
#endif

_NATIVE_WCHAR_T_DEFINED is defined iff /Zc:wchar_t- is passed which is apparently a bad thing to do.

change the declaration of WCHAR from wchar to wchar_t

We should probably do that anyway, but it won't fix the problem, only move it elsewhere.

@WalterBright
Copy link
Member

Putting in the cast to wchar_t* will at least inure the module against whatever solution we go with.

@WalterBright
Copy link
Member

This suggests a solution for us.

  1. define WCHAR as wchar_t
  2. for version(Cpp98), define wchar_t the old way
  3. otherwise define wchar_t the new way
  4. uses casts to ensure druntime/Phobos compiles either way
  5. people whose code breaks can use -transition=cpp98 until their code is transitioned

@WalterBright
Copy link
Member

See dlang/dmd#9029 for addition of switch.

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed stalled labels May 27, 2021
@Geod24
Copy link
Member

Geod24 commented Jun 2, 2021

Closing as it hasn't seen any activity for a while and is outdated (also cleaning up branches in the repository).

@Geod24 Geod24 closed this Jun 2, 2021
@Geod24 Geod24 deleted the thewilsonator-patch-1 branch June 2, 2021 03:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Needs Rebase needs a `git rebase` performed stalled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants