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

Comments

Add musl libc definitions to: src_core_thread#2007

Merged
dlang-bot merged 1 commit intodlang:masterfrom
yshui:musl_libc_src_core_thread
Jan 12, 2018
Merged

Add musl libc definitions to: src_core_thread#2007
dlang-bot merged 1 commit intodlang:masterfrom
yshui:musl_libc_src_core_thread

Conversation

@yshui
Copy link
Contributor

@yshui yshui commented Dec 26, 2017

Splitted from #1997

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @yshui! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

@joakim-noah
Copy link
Contributor

No need for a separate pull for every file, a good way to split it is three pulls, like the recent DragonFlyBSD pulls. I'll look at these file pulls for now- small is fine- but for the rest of your original pull, splitting it like the DragonFly pull is a good idea.

@yshui
Copy link
Contributor Author

yshui commented Dec 26, 2017

No need for a separate pull for every file

@joakim-noah I already did...

@yshui yshui changed the base branch from stable to master December 26, 2017 05:54
@joakim-noah
Copy link
Contributor

Yes, you already split it by file for some of your original massive pull #1997, but for the rest, please just split it in two, not every file by itself.

@wilzbach wilzbach force-pushed the musl_libc_src_core_thread branch from 475cf47 to d355984 Compare December 29, 2017 16:28
@wilzbach
Copy link
Contributor

Rebased to master fix the CircleCi & DAutoTest failures.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 12, 2018

All these are just additions and look reasonable to me. One pull request per package (core.stdc, core.sys, etc) is the usual thing to do for ease of review.

@dlang-bot dlang-bot merged commit 246d53c into dlang:master Jan 12, 2018
@yshui
Copy link
Contributor Author

yshui commented Jan 12, 2018

@ibuclaw @joakim-noah So it's ok to submit one PR per module?

@wilzbach
Copy link
Contributor

Yes, your initial PR was just a bit too large to be reviewed in one piece.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 12, 2018

@yshui - Yes, of course its fine, although 4 PRs would have been fine also. One for core/stdc/*, one for core/sys/*, one for the rest of core/*, and one for rt/*.

@joakim-noah
Copy link
Contributor

One pull per module is way too small for changes like this, please just remove all these merged patches from your original pull #1997 and we'll get that reduced patch in.

@yshui
Copy link
Contributor Author

yshui commented Jan 13, 2018

@joakim-noah OK. I'm just waiting for all the submitted patches to merge before submitting the rest

@joakim-noah
Copy link
Contributor

No, don't submit the rest separately. Just trim down #1997 of the parts that were already merged, and we'll get that first pull in.

@yshui
Copy link
Contributor Author

yshui commented Jan 13, 2018

@joakim-noah I meant to say that I'll submit the rest splitted into 4 parts after the outstanding PRs are merged.

@joakim-noah
Copy link
Contributor

If you like, but simply removing these merged patches from #1997 should be enough to get that one in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants