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

Switch maplike/setlike to use infra maps and sets #1138

Merged
merged 18 commits into from
May 5, 2022

Conversation

tabatkins
Copy link
Contributor

@tabatkins tabatkins commented May 2, 2022

Fixes #254, fixes #824.

This PR switches maplike and setlike interfaces from using a [[BackingMap]]/[[BackingSet]] ES Map/Set to use an Infra map/set instead. As explained in #254 and #824, this makes the interfaces vastly easier to work with in spec-ese (you can directly manipulate an Infra map/set, rather than having to carefully perform the ES algo dance) , and removes some underdefined behavior (as currently defined, the algorithms look up and utilize the equivalent methods on Map.prototype and Set.prototype, which can be author-supplied).

Unless the author is relying on the "replace the Map.prototype methods" behavior (I haven't tested if impls actually allow this, in any case), this change should be undetectable, and just result in specs using maplike or setlike interfaces having an easier, better-defined time manipulating them.

Progress:

  • Switched maplike and setlike to use an Infra map/set instead of a [[BackingMap]]/[[BackingSet]]
  • Switched @@iterator to go over the infra map/set (but still return an iterator with the right prototype)
  • Switch the rest of the Map methods to use the infra map
  • Switch the rest of the Set methods to use the Infra set
  • Moved the maplike/setlike @@iterator and forEach methods to live with the rest of their methods, rather than being in their own section
  • Add verbage for "map setting steps"/etc for specs to use when defining custom behavior, a la "getter steps"

Preview | Diff

@tabatkins
Copy link
Contributor Author

Ugh, I didn't push all my latest commits from my work machine. This is even more WIP than intended; you'll have to wait for tomorrow to actually see my current progress.

@tabatkins
Copy link
Contributor Author

Okay, current progress is up and now matches the task list as I have represented it. Review would be nice, but I'll continue working on this without waiting.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Generally looks like it's on the right track; very nice work.

I only left a few substantive comments. I think there are some editorial things but we can do those in a final pass. One more-pervasive editorial thing you might want to get done earlier is to use "string" instead of "string" for strings.

@tabatkins tabatkins marked this pull request as ready for review May 3, 2022 20:42
@tabatkins
Copy link
Contributor Author

Okay, finished the work I wanted to do, and addressed @domenic's editorial concerns they've raised so far. This PR is ready for proper review now.

I'm putting off defining "map setting steps"/etc; the text already defines that specs that want to provide their own set/delete/etc should just provide normal IDL methods of those names, so that should suffice.

I'd like to, probably in a follow-on issue, define all these built-ins in terms of the existing "create an operation function" algo, as that does all the nice setup that I'm currently doing by hand in each of the definitions, but that probably requires rejiggering the whole thing into imperative style. (I was gonna write an abstraction algo that folded away most of the boilerplate, but it turns out that "create an operation function" is that abstraction algo.) The current style is how the spec currently defines these operations, so keeping it as-is will at least not make things worse for now.

Copy link
Member

@domenic domenic 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 do a full review of setlike but there might be some stuff to copy over.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This round of editorial fixes seems likely final-ish...

@tabatkins
Copy link
Contributor Author

Oof, sorry about the lingering copy-paste errors I still had.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. @EdgarChen do you want to take a look? I feel like I understand the map/set/infra parts of the Web IDL spec pretty well but as editor you should definitely get a chance to weigh in if you want :)

@EdgarChen
Copy link
Member

I took a look and it looks good to me. Thanks for all your works!

@Liamolucko
Copy link

This PR changed the setlike add method to return the value that was added to the setlike object instead of the object itself. Was that an accident?

@TimothyGu TimothyGu changed the title WIP: switching maplike/setlike to use infra maps and sets Switch maplike/setlike to use infra maps and sets Feb 16, 2023
@TimothyGu
Copy link
Member

@tabatkins Can you check the comment above?

@annevk
Copy link
Member

annevk commented Feb 17, 2023

When PRs or issues are closed, please always file a follow-up. I've taken care of it this time around: #1268.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants