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

chaperone-treelist fixes and doc edits #4982

Merged
merged 2 commits into from May 13, 2024

Conversation

LiberalArtist
Copy link
Contributor

@LiberalArtist LiberalArtist commented Apr 25, 2024

This PR has two bug fixes and several documentation edits related to chaperone-treelist.

The bugs:

  1. 4530cd9 documented that #:ref could be #f, but check-chaperone-arguments didn't actually allow it. This is fixed in ed70e7f.
  2. The documented contract for #:append2 had the wrong number of arguments (i.e. one of the state arguments was missing).

I would be happy to split this PR up further if either of those fixes seem important enough to put into 8.13 at this late stage: I only wish I had discovered them sooner!

The other non-documentation change in this PR is that I made #:ref optional.

Several things that I have not done (yet?):

  1. At one point I considered whether #:state should also be optional, but I decided against it: if there were really a use-case for chaperoning for which state was irrelevant, it would be better to provide an alternate function that wouldn't need the corresponding arguments and return values, either.
  2. I'd like to do something about places that say procedures must accept @racket[tl] and produce results chaperoned with the same procedures and properties as @racket[tl]. The argument tl is without the layer of chaperoning added by this call to chaperone-treelist, so treelist-chaperone-state and impersonator-property accesses on that tl won't work. Perhaps pedantically, the argument tl might also be a derived treelist, rather than the tl from the formal arguments to chaperone-treelist. That also means the result treelist more precisely gets "the same procedures and properties" added by this call to chaperone-treelist, or something like that. But I haven't yet figured out a concise way of writing (the important parts of) that.
  3. In the example I added, tl and state are italicized as though by @var{}, whereas pos, v, other, and other-state are not. They could easily all be italicized via _, but it might be more correct to italicize none of them.
  4. After the paragraph beginning When two chaperoned treelists are given to @racket[treelist-append] and @racket[append2-proc] is not used, I'd like to say more about what happens when append2-proc is used, but I'm still confirming that I understand the details I think I want to write.

More generally I have some observations now that I've (finally) experimented with the improved treelist chaperoning a bit, but I probably won't finish writing those up tonight and, having found the bugs here, I didn't want to delay reporting them. (In brief, this still seems like a fine direction.)

@mflatt
Copy link
Member

mflatt commented Apr 25, 2024

I think the better fix for #:ref as #f is probably to remove that option from the contract in the docs, and then also remove the unneeded branch in the implementation. A #f was allowed at one point to enable efficient treelist-append, but the addition of more append hooks addressed that problem more completely.

  * An `append2-proc` must accept four arguments, not three.

  * Do not allow `#f` for `ref-proc`.

  * Move discussion of `delete-proc`, `take-proc`, and `drop-proc`
    before `append-proc`, `prepend-proc`, and `append2-proc`,
    on the grounds that operations on one treelist are relatively
    similar, whereas the two-treelist operations get into some
    extended discussion.

  * Specify that `state-key` is recognized by `eq?`.

  * Add example for `chaperone-treelist`.

  * The result of `impersonate-mutable-treelist` is a non-chaperone
    impersonator.

  * Typo fix.
Calling `chaperone-treelist` and friends with `#:ref #f` has not been
allowed since 4530cd9.
@LiberalArtist
Copy link
Contributor Author

I think the better fix for #:ref as #f is probably to remove that option from the contract in the docs, and then also remove the unneeded branch in the implementation.

That makes sense. I've pushed a revised series with a docs-only patch, then the implementation change. I changed chaperone-mutable-treelist and impersonate-mutable-treelist to match, and in the process noticed that chaperone-mutable-treelist should be documented as returning a non-chaperone impersonator.

In the example I added, tl and state are italicized as though by @var{}, whereas pos, v, other, and other-state are not. They could easily all be italicized via _, but it might be more correct to italicize none of them.

I used _, since it was easy, for the sake of at least local consistency.

@mflatt mflatt merged commit cfa05db into racket:master May 13, 2024
7 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.

None yet

2 participants