Skip to content
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

Add support for '-platformlib' option. Fixes #2031 #2032

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

dbankov-vmware
Copy link
Contributor

@dbankov-vmware dbankov-vmware commented Nov 4, 2020

This change adds the newly introduced LDC linker option '-platformlib'
to the isLinkeDFlag(...) function in ldc.d so the option to be passed to
LDC when invoked by DUB for linking.

@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @dbankov-vmware! 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 coverage diff by visiting the details link of the codecov check)
  • 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.

@dbankov-vmware dbankov-vmware changed the title Fix for issue #2031 Fixes #2031 Nov 4, 2020
@Geod24
Copy link
Member

Geod24 commented Nov 4, 2020

Please give a proper title and description to both PR and commits.

Remember that:

  1. Many people might "Watch" this repository, and a title like "Fix issue 123" means that they have to open the notification to see if that's something they are interested / competent in.
    Now multiply that by all the watchers and all the PRs that are submitted to this repository, and you quickly see how saving a minute on properly explaining changes wastes time for everyone else.

  2. Not only the Github notification matters, but the git history as well. Contributors routinely use git blame or other means of displaying the history while working on this project. It's an invaluable help when developing new features (finding the reason a piece of code is here) or finding regressions (see if the commit could potentially affect the observed behavior) alike.

Our contributing guidelines include an article about good commit messages which is well worth the read.

This change adds the newly introduced LDC linker option '-platformlib'
to the isLinkeDFlag(...) function in ldc.d so the option to be passed to
LDC when invoked by DUB for linking.
@dbankov-vmware
Copy link
Contributor Author

Updated title and description for both the PR and the commit.
I looked at https://github.com/dlang/dub/blob/master/CONTRIBUTING.md before filing the PR which referred to https://github.blog/2013-01-22-closing-issues-via-commit-messages/ which left me with the impression that the commit message should include only 'Fixes...'.

@Geod24 Geod24 changed the title Fixes #2031 Add support for '-platformlib' option. Fixes #2031 Nov 8, 2020
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Thanks!

I looked at https://github.com/dlang/dub/blob/master/CONTRIBUTING.md before filing the PR which referred to https://github.blog/2013-01-22-closing-issues-via-commit-messages/ which left me with the impression that the commit message should include only 'Fixes...'.

Thanks for the feedback, I'll look at updating the contributing guide to make requirements clearer.

@Geod24 Geod24 merged commit 3b31d99 into dlang:master Nov 8, 2020
@dbankov-vmware dbankov-vmware deleted the fix-for-2031 branch November 8, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants