-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Fix for #3374 #3475
Fix for #3374 #3475
Conversation
driver/linker-msvc.cpp
Outdated
@@ -85,6 +94,26 @@ void addSanitizerLibs(std::vector<std::string> &args) { | |||
|
|||
////////////////////////////////////////////////////////////////////////////// | |||
|
|||
static std::vector<std::string> getPlatformLibNames() { |
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.
Thx for following up. - Please move this to linker.cpp
(perhaps as getExplicitPlatformLibs()
returning an llvm::Optional<std::vector<string>>
) as we'll support this option for Posix targets too (see here). The tokenizing can then be extracted as little helper, used for both -defaultlib
and -platformlib
.
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.
Finally got some time to do this. I'm sorry for the delay.
Please take a look and let me know if I should change something.
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.
No worries. - I've pushed a commit, slightly revising a few things, extending it to Posix targets too as well as extending the tests as requested by Johan.
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.
Thank you very much Kinke!
I was going to take care of this over the weekend as I've been quite busy at work lately...
69ca325
to
d2fc955
Compare
Introduce -platformlib option to control libraries added automatically for Windows targets.
d2fc955
to
ccaccb8
Compare
tests/linking/platformlib.d
Outdated
* -mtriple=x86_64-unknown-windows-coff. | ||
*/ | ||
|
||
// RUN: not %ldc -v -mtriple=x86_64-unknown-windows-coff -platformlib= %s | FileCheck %s |
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.
can you also add a testcase where you pass some strings through the new option?
[I've apparently hit some GitHub bug while merging this - this shouldn't have been a squash-merge, plus I've customized the merge commit msg - then got an error and pressed a retry button, which squash-merged it without further confirmation...] |
I'm not particularly proud of my part of the change so IMO it is better that things happened this way :). |
Well, if it wasn't for you and your initiative, we wouldn't have this. :) - This wasn't a squash merge, but a normal merge with the actual merge commit (title + msg) looking like a squash-merge-commit... |
What exactly is the difference between |
IIRC, mainly that |
Introduce -platformlib option to control libraries added automatically for Windows targets.