Skip to content
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

Introduce move-inhibiting version of KJ_DISALLOW_COPY #236

Merged
merged 4 commits into from
Dec 22, 2022

Conversation

harrishancock
Copy link
Collaborator

@harrishancock harrishancock commented Dec 22, 2022

This commit is not intended to be merged as-is, because it is currently a source-incompatible change. Rather, it is intended to prove that we have retrofitted all existing uses of KJ_DISALLOW_COPY to only appear in cases where move semantics are not required.

A later commit will rename KJ_DISALLOW_COPY to KJ_DISALLOW_COPY_AND_MOVE, and then rename KJ_DISALLOW_ONLY_COPY back to KJ_DISALLOW_COPY, making this a source-compatible change.

Edit: the later commit has been pushed, and capnproto dependency updated. Ready to merge.

@harrishancock harrishancock force-pushed the harris/move-inhibiting-disallow-copy branch from d451167 to a0382d0 Compare December 22, 2022 18:57
@harrishancock harrishancock marked this pull request as ready for review December 22, 2022 19:29
@harrishancock harrishancock force-pushed the harris/move-inhibiting-disallow-copy branch from f384df7 to 355aad1 Compare December 22, 2022 20:05
This is needed to pick up the new KJ_DISALLOW_COPY_AND_MOVE macro.
This commit is not intended to be merged as-is, because it is currently a source-incompatible change. Rather, it is intended to prove that we have retrofitted all existing uses of KJ_DISALLOW_COPY to only appear in cases where move semantics are not required.

A later commit will rename KJ_DISALLOW_COPY to KJ_DISALLOW_COPY_AND_MOVE, and then rename KJ_DISALLOW_ONLY_COPY back to KJ_DISALLOW_COPY, making this a source-compatible change.
This converts the previous commit to a source-compatible change, and introduces a better name for the move-inhibiting version of this macro: KJ_DISALLOW_COPY_AND_MOVE.
My first pass was very mechanical, and I missed that these could be improved.
@harrishancock harrishancock force-pushed the harris/move-inhibiting-disallow-copy branch from c1ef0a4 to 2343db5 Compare December 22, 2022 20:30
@kentonv
Copy link
Member

kentonv commented Dec 22, 2022

Seems like the commit messages may need updating (one says it shouldn't be merged) and maybe the commits should be squashed?

@kentonv
Copy link
Member

kentonv commented Dec 22, 2022

Seems like the commit messages may need updating (one says it shouldn't be merged) and maybe the commits should be squashed?

Eh I don't care that much, up to you.

@harrishancock
Copy link
Collaborator Author

Normally I would agree and squash these down, now that the change is stable. However, since this is a change across three different repos, our CIs take a long time, and the probability of becoming conflicted is relatively high, I'm going to merge while CI is green.

@harrishancock harrishancock merged commit ee684a8 into main Dec 22, 2022
@harrishancock harrishancock deleted the harris/move-inhibiting-disallow-copy branch December 22, 2022 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants