-
-
Notifications
You must be signed in to change notification settings - Fork 695
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
Eliminate shared this from std/process.d #5466
Eliminate shared this from std/process.d #5466
Conversation
std/process.d
Outdated
} | ||
else | ||
{ | ||
// Made available by the C runtime: | ||
extern(C) extern __gshared const char** environ; | ||
private extern(C) extern __gshared const char** environ; | ||
private const(char**) getEnvironPtr() @property @trusted |
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.
Nit, get
implies an action, so having the name start with get
should be mutually exclusive with @property
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.
BTW, as all this is in a private { }
block, the private
annotations here are redundant.
Oh, unindented @CyberShadow I rebeled by changing the indent, please don't make me create another PR for it. Thanks. |
IMO it's fine as long as it's a separate commit. |
Though, ideally, whitespace changes should be in their own commit. |
std/process.d
Outdated
{ | ||
// Made available by the C runtime: | ||
extern(C) extern __gshared const char** environ; | ||
const(char**) getEnvironPtr() @property @trusted |
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.
getXXXXX
and @property
? One or the other I think.
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.
done
LGTM overall. |
1c23ab8
to
5722113
Compare
5722113
to
ddfe6ad
Compare
@andralex Took the liberty of doing this myself. |
Should be fairly trivial do as a Dscanner plugin. I might be able to have a look at this tonight ;-) |
@CyberShadow don't forget that a security feature, we remove auto-merge if the content of the PR changes. |
Oh, I didn't see that Andrei added it. The PR has been open only 3 hours, and I think we should allow 24 hours for review of non-trivial PRs. |
Have a look at #5477 and dlang-community/D-Scanner#441 |
LGTM. It's kind of embarrassing that I didn't implement it like this in the first place... |
LGTM. Fix the merge conflict, and I think we are good. |
ddfe6ad
to
5ac8ce7
Compare
Done. No need to wait on Andrei for resolving merge conflicts - this actually didn't even require manual interaction ;-) |
5ac8ce7
to
3b3cd77
Compare
@wilzbach thanks, any idea bout the circleci failure? I can't understand what the problem there is. |
Seems like a problem with DUB - unfortunately the DUB registry isn't that stable (e.g. dlang/dub#1104). We really should setup a couple of mirror servers ... |
Alrighty, let's do this! |
Why didn't auto-merge work? The label was added hours ago. |
The log for the last hours looks like this - the bot tried really hard:
And the context for one event looks like this:
Maybe there's sth. weird with the GitHub API going on (e.g. it doesn't allow API merges if a maintainer has force-pushed the PR and it isn't reviewed yet)? |
It's probably DLang-Bot's fault to send the wrong sha -> dlang/dlang-bot#77 |
cc @kyllingstad @CyberShadow @John-Colvin @braddr @markisaa @wilzbach @joakim-noah