Skip to content

Conversation

@witch-factory
Copy link
Contributor

@witch-factory witch-factory commented Mar 11, 2024

@witch-factory witch-factory requested a review from a team as a code owner March 11, 2024 01:58
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is meant to be interpreted as zero indexed. Documenting that seems reasonable though. Maybe "All non-numeric (bool, null, undefined) immediates have bit 1 (zero indexed) set."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. However, there are sections in other comments as well where the bit ordering is represented as 0-indexed. So how about making a similar documenting to those part? This would help clarify that the bits are 0-indexed in the content of the subsequent comments as well.

Line 454~458:

     * These values have the following properties:
     * - Bit 1 (0-indexed) is set (OtherTag) for all four values, allowing real pointers to be
     *   quickly distinguished from all immediate values, including these invalid pointers.
     * - With bit 3 (0-indexed) masked out (UndefinedTag), Undefined and Null share the
     *   same value, allowing null & undefined to be quickly detected.

Line 485 as reviewed.

// All non-numeric (bool, null, undefined) immediates have bit 1 (0-indexed) set.

The expression 0-indexed are being used in Source/JavaScriptCore/runtime/JSBigInt.cpp

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

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

I think your commit comment is duplicated.

@witch-factory
Copy link
Contributor Author

I apologize for the mistake in handling Git. I have now combined the commit messages into one. Could you please take a look?

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

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

r=me.

@kmiller68 kmiller68 added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Mar 11, 2024
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 12, 2024
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #14672.

https://bugs.webkit.org/show_bug.cgi?id=270775

Reviewed by Keith Miller.

It is the second bit which is set for non-numeric immediates and the second bit is bit 1.

* Source/JavaScriptCore/runtime/JSCJSValue.h:

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

Committed 275951@main (b2b0e19): https://commits.webkit.org/275951@main

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

@webkit-commit-queue webkit-commit-queue merged commit b2b0e19 into WebKit:main Mar 12, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 12, 2024
@witch-factory witch-factory deleted the eng/Fix-typos-in-comments-of-JSCJSValue-h branch March 12, 2024 02:56
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.

5 participants