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

Comments

[Trivial] Add dragonfly implementation for shared memory#2106

Merged
dlang-bot merged 1 commit intodlang:stablefrom
dkgroot-dlang:fix_shm
Feb 23, 2018
Merged

[Trivial] Add dragonfly implementation for shared memory#2106
dlang-bot merged 1 commit intodlang:stablefrom
dkgroot-dlang:fix_shm

Conversation

@dkgroot
Copy link
Contributor

@dkgroot dkgroot commented Feb 23, 2018

This one was overlooked (and noticed by @joakim-noah in backporting to ldc-ltsmaster)

@dlang-bot dlang-bot added the Trivial typos, formatting, comments label Feb 23, 2018
@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 23, 2018

Thanks for your pull request and interest in making D better, @dkgroot! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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.

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.

Copy link
Contributor

@joakim-noah joakim-noah left a comment

Choose a reason for hiding this comment

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

Properly scoped, LGTM.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Don't you want to target stable?

time_t shm_atime;
time_t shm_dtime;
time_t shm_ctime;
void * __shm_internal;
Copy link
Contributor

Choose a reason for hiding this comment

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

DStyle: void* (also do you have any resource where it's __shm_internal. I did a quick search, but could only find shm_internal like it's for FreeBSD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Dfly, /usr/include/sys/shm.h looks like:

struct shmid_ds {
    struct ipc_perm shm_perm;   /* operation permission structure */
    size_t          shm_segsz;  /* size of segment in bytes */
    pid_t           shm_lpid;   /* process ID of last shared memory op */
    pid_t           shm_cpid;   /* process ID of creator */
    shmatt_t    shm_nattch; /* number of current attaches */
    time_t          shm_atime;  /* time of last shmat() */
    time_t          shm_dtime;  /* time of last shmdt() */
    time_t          shm_ctime;  /* time of last change by shmctl() */
    void           *shm_internal;   /* sysv stupidity */
};

Have not looked at the use or implementation of this header file. I did have to gloss over the comment for this pointer (Logically it would be the opaque pointer to an internal representation/expansion for this struct,). But i thought size might matter (doesn't it always).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.
I would suggest to keep the shm_internal name then. You could set it to private though?

But i thought size might matter

Yes.

@dkgroot
Copy link
Contributor Author

dkgroot commented Feb 23, 2018

@wilzbach > Don't you want to target stable?
Sorry i don't understand, what do you mean ?

@wilzbach wilzbach changed the base branch from master to stable February 23, 2018 18:28
@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Feb 23, 2018
@wilzbach
Copy link
Contributor

Sorry i don't understand, what do you mean ?

The master -> stable branch-off already happened. If you target stable, it will be part of the 2.079 tag.

@wilzbach
Copy link
Contributor

For reference, when retargeting a PR to stable it needs to be rebased locally too:

git rebase --onto upstream/stable upstream/master

Also if you depend on this in master, you can simply run merge_stable yourself and get the changes back into master.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

FYI: I squashed your PR + fixed the DStyle (void* not void *)

@dlang-bot dlang-bot merged commit 6dd5d20 into dlang:stable Feb 23, 2018
@dkgroot
Copy link
Contributor Author

dkgroot commented Feb 23, 2018

Thanks @wilzbach / @joakim-noah

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

Labels

Bug Fix Include reference to corresponding bugzilla issue Trivial typos, formatting, comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants