Skip to content

Document [row_more] and [row_fixed].#14023

Merged
gasche merged 4 commits intoocaml:trunkfrom
goldfirere:document-row-more
May 14, 2025
Merged

Document [row_more] and [row_fixed].#14023
gasche merged 4 commits intoocaml:trunkfrom
goldfirere:document-row-more

Conversation

@goldfirere
Copy link
Copy Markdown
Contributor

As discussed earlier this week. Review request from @garrigue (or perhaps @gasche).

@goldfirere goldfirere requested a review from garrigue May 9, 2025 18:53
@ocaml ocaml deleted a comment May 10, 2025
@ocaml ocaml deleted a comment May 10, 2025
@gasche
Copy link
Copy Markdown
Member

gasche commented May 10, 2025

(It looks like this issue attracts automated spammers for some reasons. Unfortunately I have no better idea than to delete spam comments after they have been posted, so I'm doing just that.)

@gasche
Copy link
Copy Markdown
Member

gasche commented May 14, 2025

@garrigue can you confirm that what's written is not factually wrong? I can take the rest of the review from that assessment, or just merge.

Copy link
Copy Markdown
Contributor

@garrigue garrigue left a comment

Choose a reason for hiding this comment

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

Looks essentially correct, except for the Tnil case.
For historical reasons, I do not think we rely on Tnil being set correctly, but it is better to have new code comply.
There is also a bit of nitpicking about the distinction between structural and physical equality; unfortunately in ocaml physical equality is observable, which makes discussions about equality more complex.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I didn't review this in-depth myself, but I'm all for better documentation of the internals, so I approve based on @garrigue's review and my trust in @goldfirere's fixes/updates.

@gasche gasche merged commit 8d9b5ed into ocaml:trunk May 14, 2025
24 checks passed
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.

3 participants