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

Add coders for std::array and WTF::EnumeratedArray #5668

Merged
merged 1 commit into from Oct 24, 2022

Conversation

@heycam heycam requested a review from cdumez as a code owner October 22, 2022 04:53
@heycam heycam self-assigned this Oct 22, 2022
@heycam heycam added WebKit Nightly Build WebKit Process Model Bugs related to WebKit's multi-process architecture labels Oct 22, 2022
Comment on lines -48 to -50
// We add 1 to the size because we expect that LastValue is the maximum value of the enum,
// rather than the count of items in the enum.
// We're assuming the values in the enum are zero-indexed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that we assume the values are zero-based, and that LastValue is indeed the last value and not one past the last value, are already mentioned in the comment at the top of the class, a few lines up. I think the explanation of why we need to add 1 is reasonably self-evident, but I'm happy to leave it in if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. I'm the author of that comment so I think I'm biased :P

{
std::array<std::optional<T>, size> items;

for (auto& item : items) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, that's nifty. The caller indicates how many items they expect, so there isn't a need to send the size across the wire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's encoded in the type itself.

@heycam heycam added the merge-queue Applied to send a pull request to merge-queue label Oct 24, 2022
https://bugs.webkit.org/show_bug.cgi?id=246898
<rdar://problem/101453146>

Reviewed by Myles C. Maxfield.

* Source/WTF/wtf/EnumeratedArray.h:
* Source/WTF/wtf/Forward.h:
* Source/WebKit/Platform/IPC/ArgumentCoders.h:

Canonical link: https://commits.webkit.org/255901@main
@webkit-commit-queue
Copy link
Collaborator

Committed 255901@main (10f809a): https://commits.webkit.org/255901@main

Reviewed commits have been landed. Closing PR #5668 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 10f809a into WebKit:main Oct 24, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
4 participants