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

Make ContextId implement Hash and simplify the code #377

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

Enselic
Copy link
Collaborator

@Enselic Enselic commented Oct 6, 2021

I thought it would be a good idea to split up #374 into smaller and self-contained parts that are easier to digest and make sense of. Here is the first such PR. It passes

  • syntect regression tests
  • bat regression tests

and I know from working with the referenced PR that the code is very sensitive, and the slightest mistake will practically always make at least one test fail. So that all tests pass gives good confidence that the code is in order.

As for the change itself; making ContextId implement Hash allows us to make the code simpler and clearer. Most importantly:

  • We don't need to pass around a cryptic HashSet<usize>. We can instead pass
    around the semantically much clearer HashSet<ContextId>,

But I also take the opportunity to:

  • Remove the ContextId::index() getter. ContextId shall be seen as a
    primitive that we pass around, rather than something with getters.

  • Stop using ContextId::new(). The way we use it, it is just boilerplate.
    (On a related note: Long-term, we should even remove it from the public API. Clients should never
    create their own ContextIds.)

* We don't need to pass around a cryptic `HashSet<usize>`. We can instead pass
  around the semantically much clearer `HashSet<ContextId>`,

* Remove the `ContextId::index()` getter. `ContextId` shall be seen as a
  primitive that we pass around, rather than something with getters.

* Stop using `ContextId::new()`. The way we use it, it is just boilerplate.
  Long-term, we should remove it from the public API. Clients should never
  create their own `ContextId`s.
Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

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

Sorry for not reviewing this for so long, my life has been busy lately. This looks good!

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

3 participants