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

Use __c_wchar_t.#2349

Closed
thewilsonator wants to merge 1 commit intomasterfrom
thewilsonator-patch-1
Closed

Use __c_wchar_t.#2349
thewilsonator wants to merge 1 commit intomasterfrom
thewilsonator-patch-1

Conversation

@thewilsonator
Copy link
Copy Markdown
Contributor

No description provided.

@dlang-bot
Copy link
Copy Markdown
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"

Comment thread src/core/stdc/config.d
@thewilsonator
Copy link
Copy Markdown
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.

Comment thread src/core/stdc/config.d
@ibuclaw
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@thewilsonator
Copy link
Copy Markdown
Contributor Author

thewilsonator commented Nov 13, 2018

... or not.

@thewilsonator
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/core/sys/windows/winnt.d Outdated
@rainers
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@thewilsonator
Copy link
Copy Markdown
Contributor Author

this needs
dlang/dmd#8958

@WalterBright
Copy link
Copy Markdown
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
Copy Markdown
Member

#2389

@WalterBright
Copy link
Copy Markdown
Member

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

@WalterBright
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@thewilsonator
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@WalterBright
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

8 participants