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

dedup dependencies #293

Merged
merged 5 commits into from
Apr 9, 2024
Merged

dedup dependencies #293

merged 5 commits into from
Apr 9, 2024

Conversation

kamadorueda
Copy link
Contributor

@kamadorueda kamadorueda commented Apr 2, 2024

Goal

Generate TypeLists containing less duplicates.

Mentions #291 #289

Changes

This is an Internal API change, use a HashSet instead of a Vec, and delay formatting until just when it's needed.

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 2, 2024

Nice! Pretty much the same as #292, but I think I like this one more.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

I explored if generating a custom TypeList impl (which would eliminate the recusion problem completely) would a sensible alternative.

It's definetely possible, but I think it'd require a breaking change to be nice. What I had in mind was

fn dependency_types() -> impl TypeList {
  #[derive(Copy, Clone)]
  struct Deps;
  impl TypeList for Deps {
    fn for_each(self, v: &mut impl TypeVisitor) {
      v.visit::<TypeOfField1>();
      v.visit::<TypeOfField2>();
      Something::dependency_types().for_each(v);
      SomethingElse::generics().for_each(v);
    }
  }
  Deps
}

This breaks with generic parameters though. The naive for_each impl would use them, but it can't. to make that work, Deps would need to be generic as well. Definetely something we could do right now, but it's kinda messy.

If we changed fn dependency_types() -> impl TypeList to fn dependency_types(v: &mut impl TypeVisitor), these problems should go away. That would remove one level of indirection.

So tl;dr: I think we should go ahead with this, and keep this change in mind if someone still runs into the issue or for a breaking 9.0 release some time in the future.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 8, 2024

Note to myself: Add two tests from other branch.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 9, 2024

So, I've added some reference counting (probably doesn't have a big performance impact, but it felt like the right thing to do), added the test from the other branch and used a HashSet for types as well.

From my side, this is ready to merge.

@escritorio-gustavo escritorio-gustavo merged commit 8cfc3d7 into Aleph-Alpha:main Apr 9, 2024
7 checks passed
@NyxCode
Copy link
Collaborator

NyxCode commented Apr 9, 2024

Thanks @kamadorueda for getting this of the ground!

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