Add DragonFlyBSD support : src/core/sys/posix related changes by dkgroot · Pull Request #2001 · dlang/druntime

dkgroot · 2017-12-19T14:56:07Z

Split PR : #1999
Moved the src/core/sys/posix related changes into this PR

dlang-bot · 2017-12-19T14:56:08Z

Thanks for your pull request, @dkgroot! 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

Skimmed for basic formatting issues and to make sure everything was scoped to version(DragonFlyBSD), found only one spurious newline removal worth changing.

joakim-noah · 2017-12-19T16:09:21Z

src/core/sys/posix/sys/socket.d

         SO_NOSIGPIPE    = 0x0800,          /* no SIGPIPE from EPIPE */
         SO_ACCEPTFILTER = 0x1000,          /* there is an accept filter */
         SO_TIMESTAMP    = 0x2000,          /* timestamp received dgram traffic */
-


No reason to take this out.

joakim-noah · 2017-12-31T10:47:09Z

Ping, @wilzbach, checked that everything was scoped to DragonFly but not for correctness. There was only a single comment on these headers when put up for review before.

wilzbach

Yep, everything is scoped nicely in DragonFlyBSD blocks. However, I am rather new to the porting process, so I am leaving the decision to more experienced members.
Nevertheless I agree with you that we can't check for correctness here.

wilzbach · 2017-12-31T10:54:13Z

src/core/sys/posix/signal.d

 }
+else version( DragonFlyBSD )
+{
+    enum SIG_HOLD = cast(sigfn_t2) 3;


enum sigfn_t2

@wilzbach Can you clarify what you want me to do ?

He's saying to just set the type of the enum, rather than casting the value. Does that work for a function pointer like this?

Done... Thanks !
Note: if this is an important change, the other version(OS) entries do perform the same cast, so you might want to make a similar change to the other implementations (in a separate PR).

CyberShadow · 2018-01-05T21:00:31Z

src/core/sys/posix/signal.d

-    enum SIG_CATCH = cast(sigfn_t2) 2;
-    enum SIG_HOLD = cast(sigfn_t2) 3;
+    enum SIG_CATCH = 2;
+    enum SIG_HOLD = 3;


I think Sebastian meant to change this to enum sigfn_t2 SIG_CATCH = 2;, i.e. move the type from the cast to the enum declaration, but not sure if it makes any difference.

@CyberShadow Thanks for reviewing the code :-)
I just made that change (10 minutes ago).

I just made that change (10 minutes ago).

Unless you're talking about something else which you haven't pushed yet, then my comment was about the code you did just push. There is no type declaration in it. I don't know if it matters or not, I just wanted to clarify on Sebastian's behalf.

To clarify even further, your code goes:

enum SIG_CATCH = 2;

I believe the suggested change was:

enum sigfn_t2 SIG_CATCH = 2;

Hopefully that should clear things up.

(Please: Do note that sigfn_f2 is a function pointer)
@CyberShadow That gives
"Error: cannot implicitly convert expression 2 of type int to extern (C) void function(int) nothrow @nogc"

This does work:
enum sigfn_t2 SIG_CATCH = cast(sigfn_t2) 2;
enum sigfn_t2 SIG_HOLD = cast(sigfn_t2) 3;

But I think i can just as well go back to to original definition in that case (using only the cast).

Ping @wilzbach , @joakim-noah and @CyberShadow
Reverted the previous patch... I think the requested change might require a little more deliberation/discussion.

Source fragment (to give some context):

nothrow @nogc { private alias void function(int) sigfn_t2; private alias void function(int, siginfo_t*, void*) sigactfn_t2; } ... else version( DragonFlyBSD ) { enum SIG_CATCH = cast(sigfn_t2) 2; enum SIG_HOLD = cast(sigfn_t2) 3; ...

The way you had it initially was fine. We weren't sure if you could just apply the type to the left, but as you found, it needs to be cast.

Thanks for the update 😉. Glad I rolled it back just before ibuclaw merged it 😙

@CyberShadow As mentioned in the dlang/druntime PR, that does not work (compiler error), i reverted it to the original version using the cast. The PR has already been merged now. If it does need to change (now or in the future), it would have to be done in a seperate PR and should also involve the other OS implementations.

Yes, I saw the earlier messages. No objections to that, of course.

ibuclaw · 2018-01-05T23:20:42Z

@dkgroot rebase / fix up?

ibuclaw

This has been approved by two other maintainers, and a quick skim looks fine to me.

dkgroot · 2018-01-06T14:00:25Z

Thanks for all your reviewing work guys !
@ibuclaw: Because this was part of a split druntime PR, this merge might have a slight side effect of causing trouble with CI's until the other two parts are also merged.

joakim-noah · 2018-01-06T15:38:45Z

The split shouldn't matter to any of our CIs, since we don't test DragonFlyBSD and these are all scoped. I'm not sure what's up with the single D project that fails, unit-threaded, guessing that's unrelated to this PR.

dkgroot · 2018-01-06T18:58:11Z

I saw some issues in the Autotester on its windows slaves complaining about these prs. Not 100% what was causing the issue.

wilzbach · 2018-01-06T19:08:34Z

Let us know about spurious failures in #ci

dkgroot · 2018-01-06T20:19:23Z

@wilzbach will do. Looks ok at the moment

dkgroot requested review from CyberShadow, MartinNowak, andralex and wilzbach as code owners December 19, 2017 14:56

joakim-noah approved these changes Dec 19, 2017

View reviewed changes

wilzbach approved these changes Dec 31, 2017

View reviewed changes

dlang-bot added Needs Work Needs Rebase needs a `git rebase` performed and removed Needs Rebase needs a `git rebase` performed labels Jan 2, 2018

dkgroot force-pushed the dragonfly-core.sys.posix branch from 32e72eb to ba08e51 Compare January 3, 2018 16:59

dlang-bot removed the Needs Work label Jan 3, 2018

dkgroot force-pushed the dragonfly-core.sys.posix branch from ba08e51 to 4cbd1b8 Compare January 3, 2018 17:30

dlang-bot added Needs Work and removed Needs Work labels Jan 4, 2018

dkgroot force-pushed the dragonfly-core.sys.posix branch from 919bc60 to 46c1fd0 Compare January 4, 2018 19:11

CyberShadow reviewed Jan 5, 2018

View reviewed changes

dlang-bot added the Needs Work label Jan 6, 2018

Add DragonFlyBSD support (src/core/sys/posix)

1e63c94

dkgroot force-pushed the dragonfly-core.sys.posix branch from 2574cca to 1e63c94 Compare January 6, 2018 12:28

dlang-bot removed the Needs Work label Jan 6, 2018

ibuclaw approved these changes Jan 6, 2018

View reviewed changes

ibuclaw added the auto-merge label Jan 6, 2018

dlang-bot merged commit c8eaa49 into dlang:master Jan 6, 2018

wilzbach mentioned this pull request Jan 9, 2018

Add a new DragonFlyBSD build host braddr/d-tester#62

Closed

Uh oh!

Comments

Conversation

dkgroot commented Dec 19, 2017

Uh oh!

dlang-bot commented Dec 19, 2017

Bugzilla references

Uh oh!

joakim-noah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakim-noah commented Dec 31, 2017

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jan 5, 2018

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joakim-noah commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkgroot commented Jan 6, 2018

Uh oh!

wilzbach commented Jan 6, 2018

Uh oh!

dkgroot commented Jan 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkgroot Jan 5, 2018 •

edited

Loading

dkgroot Jan 6, 2018 •

edited

Loading

dkgroot commented Jan 6, 2018 •

edited

Loading

joakim-noah commented Jan 6, 2018 •

edited

Loading