Skip to content

cleanup: remove readwrite.h, use unistd.h instead. - #43

Open
alanpost wants to merge 1 commit into
mainfrom
pr-remove-readwrite-h
Open

cleanup: remove readwrite.h, use unistd.h instead.#43
alanpost wants to merge 1 commit into
mainfrom
pr-remove-readwrite-h

Conversation

@alanpost

Copy link
Copy Markdown
Contributor

The header readwrite.h (re)declares read(2) and write(2). We should use the system-provided header for these declarations, unistd.h.

This patch systematically removes readwrite.h and includes unistd.h instead.

@DerDakon

Copy link
Copy Markdown
Member

Both share a common problem: they touch a lot of files, which easily will break external patches that add their own headers. I would suggest that we split this into 2 PRs, and #44 as well: one that modifies readwrite.h to just #include <unistd.h>, and one that entirely get's rid of that header. This way we can merge the first one e.g. in 1.08, adding correctness while still allowing external patches to easily be applied. And the other one could come in a later release when we decided to allow patch breakage.

@alanpost
alanpost force-pushed the pr-remove-readwrite-h branch 2 times, most recently from b9e235a to c3374c3 Compare August 6, 2019 13:58
@alanpost
alanpost force-pushed the pr-remove-readwrite-h branch 2 times, most recently from c7d7ebf to a1e7caa Compare August 24, 2019 14:37
@alanpost alanpost modified the milestones: 1.08, 1.9 Aug 24, 2019
@alanpost

Copy link
Copy Markdown
Contributor Author

I would suggest that we split this into 2 PRs, and #44 as well: one that modifies readwrite.h to just #include <unistd.h>, and one that entirely get's rid of that header.

I've done this here, and as a consequence set this PR to the 1.9 release. I've submitted PR #80 for the beginning of this series, with the minimal change as you suggest, for the 1.08 release.

read(2) and write(2) are declared in unistd.h.
@alanpost
alanpost force-pushed the pr-remove-readwrite-h branch from a1e7caa to 7a10f65 Compare August 25, 2019 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants