Skip to content

Downstreams for renaming #3828

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

voodoos
Copy link
Contributor

@voodoos voodoos commented Apr 8, 2025

  1. Distinguish uids from interfaces and implementations: [tooling] Distinct uids for interfaces ocaml/ocaml#13286

There should be a cleaner way to store the additional information. Upstream this info is part of a Unit_info.t record that is stored as the "current unit" in the environement. But here in flambda-backend it is Compilation_unit.t instead. This looks like conflicting refactorings that it would be useful to reconcile.
Here, to keep the prototype simple, we use a tuple to add that information.

  1. Linked related declarations together: [tooling] Remember linked declarations ocaml/ocaml#13308

@voodoos
Copy link
Contributor Author

voodoos commented Apr 8, 2025

@lukemaurer what do you think would be the best way to handle the additionnal information in bcfebba ?

In the upstream compiler, we now store a Unit_info.t as the "current unit", but in flambda backend it is a Compilation_unit.t which is less obviously extendable. Here, to keep the prototype simple, I used a tuple to add the intf/impl information.

@mshinwell
Copy link
Collaborator

@lukemaurer I'd like to discuss the Unit_info part of this

@voodoos voodoos force-pushed the downstreams-for-renaming branch from 6d2f7d2 to 7ca7182 Compare April 10, 2025 13:39
voodoos added 5 commits April 10, 2025 10:21
Keep merge conflicts

[tooling] Distinct uids for interfaces
[tooling] Distinct uids for interfaces

There should be a cleaner way to store the additional information. Upstream this info is
   part of a `Unit_info.t` record that is stored as the "current unit" in the
   environement. But here in flambda-backend it is `Compilation_unit.t` instead. This
   looks like conflicting refactorings that it would be useful to reconcile.

   Here, for the prototype, we use a tuple to add that information.
Keep git conflicts

[tooling] Remember linked declarations
[tooling] Remember linked declarations
@voodoos voodoos force-pushed the downstreams-for-renaming branch from 7ca7182 to cb9cf1d Compare April 10, 2025 15:10
@voodoos voodoos marked this pull request as ready for review April 10, 2025 15:22
@lukemaurer
Copy link
Contributor

Hmm. I think it would make more sense to do what we've been intending to do for a while and have Unit_info.t store a Compilation_unit.t rather than a modname, and then Env.{get,set}_unit_name can just do exactly what they do upstream. The only reason we didn't do that before is because it didn't make sense in the middle of a big merge.

(Ultimately I'm not sure that Compilation_unit.t is the right type here as opposed to Compilation_unit.Name.t, but that can be another PR.)

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