Fix Issue 19498 - undefined identifier rt_loadLibraryW #2414
Conversation
edi33416
requested review from
andralex,
MartinNowak and
wilzbach
as code owners
|
Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + druntime#2414" |
src/core/runtime.d
Outdated
| @@ -21,6 +21,8 @@ else version (WatchOS) | |||
|
|
|||
| /// C interface for Runtime.loadLibrary | |||
| extern (C) void* rt_loadLibrary(const char* name); | |||
| /// ditto | |||
| version (Windows) extern (C) void* rt_loadLibrary(const WCHAR* name); | |||
There was a problem hiding this comment.
Incompatible duplicate declarations for version (Windows) like this are rejected by LDC, and rightly so IMO. It also prevents a compile error when using the char 'overload' on Windows.
I don't think it's a good idea to have a C interface which diverges for Windows. The different param type isn't part of the mangled name, so you won't get linker errors, but ugly crashes for external (C[++]...) code using that interface (with the old const char* argument or in portable code which reasonably assumes this basic C interface is platform-independent).
There was a problem hiding this comment.
@edi33416 per #2392 (review) that should be added back as rt_loadLibraryW that uses WCHAR*
There was a problem hiding this comment.
@thewilsonator I had the same fix in mind, but please see @WalterBright comments on the issue.
There was a problem hiding this comment.
I can't move the core.runtime.loadLibrary() implementation to rt.dmain2.rt_loadLibrary(), as rt_loadLibrary() takes a const char* instead of const char[], so it's losing the length information (unless it's safe to call strlen(name) )
edi33416
force-pushed
the
issue_19498
branch
from
30811b7
to
2050da1
Compare
| buf[len] = '\0'; | ||
|
|
||
| return rt_loadLibraryW(buf); | ||
| name[$ - 1] = '\0'; |
There was a problem hiding this comment.
Is this actually tested? Given in char[] name I would expect mutation to fail.
There was a problem hiding this comment.
Yes, fails to compile. If it would, it removes the last character. Time to add a test similar to test/uuid?
|
Pedantically, this and #2392 are breaking changes:
|
|
My suggestion: just restore the declaration for now: #2425 |
|
#2425 was merged -> closing. |
No description provided.