8000 Downstreams for renaming by voodoos · Pull Request #3828 · oxcaml/oxcaml · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 24, 2025
Merged

Downstreams for renaming #3828

merged 5 commits into from
Jun 24, 2025

Conversation

voodoos
Copy link
Contributor
@voodoos voodoos commented Apr 8, 2025 •
edited
Loading
  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 voodoos force-pushed the downstreams-for-renaming branch from 75efadc to 5076bb0 Compare April 8, 2025 19:48
@voodoos voodoos mentioned this pull request Apr 8, 2025
Closed
4 tasks
@voodoos voodoos force-pushed the downstreams-for-renaming branch from 5076bb0 to 919b045 Compare April 8, 2025 21:49
@voodoos
Copy link
Contributor Author
voodoos commented Apr 8, 2025 •
edited
Loading

@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.

@voodoos voodoos force-pushed the downstreams-for-renaming branch from 29f9e96 to 6f67595 Compare April 8, 2025 22:07
@voodoos voodoos mentioned this pull request Apr 9, 2025
Open
5 tasks
@voodoos voodoos force-pushed the downstreams-for-renaming branch from 4de6e71 to 1cf2d36 Compare April 9, 2025 13:58
@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 2 times, most recently 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.)

@lukemaurer
Copy link
Contributor

Discussed this with @mshinwell. I think putting a Compilation_unit.t in Unit_info.t makes the most sense. The alternative would be to combine Compilation_unit and Unit_info, but I don't think they're really combinable: a Compilation_unit.t is a (complicated) identifier and a Unit_info.t also has things like the source file and (in 5.3) the kind field. It makes a lot of sense for Env.get_unit_name to return a rich Unit_info.t and not much sense for an identifier to be aware of whether it's from the intf or the impl.

Concretely, what I propose is:

  • Unit_info.t adds the kind field from upstream 5.3
  • Unit_info.t stores a Compilation_unit.t instead of a modname. Awkwardly, it will need two exported constructors (or some fiddly refactoring in compile_common.ml and elsewhere):
(* the existing constructor, which will need the pack prefix to create a [Compilation_unit.t] *)
val make: ?check_modname:bool -> source_file:filename -> for_pack_prefix:Compilation_unit.Prefix.t -> intf_or_impl -> file_prefix -> t

(* a new constructor, which already knows the [Compilation_unit.t] *)
val of_compilation_unit: source_file:filename -> output_prefix:file_prefix -> kind:intf_or_impl -> Compilation_unit.t -> t
  • Env.{get,set}_unit_name and the like operate on Unit_info.t, like upstream
  • Shape.Uid.mk takes a Unit_info.t option, like upstream

I think this gets us to where things make good sense now and are not very different from upstream. There's still a bit of overlap in functionality between Unit_info and Compilation_unit but that can be cleaned up later.

@lukemaurer lukemaurer mentioned this pull request Apr 24, 2025
Merged
@lukemaurer
Copy link
Contributor

Concretely, what I propose is:

This proposal is what became #3926.

@voodoos voodoos force-pushed the downstreams-for-renaming branch from cb9cf1d to c32dd7b Compare June 3, 2025 14:48
@voodoos voodoos force-pushed the downstreams-for-renaming branch from c32dd7b to 53f76f1 Compare June 3, 2025 15:51
lukemaurer
lukemaurer approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor
@lukemaurer lukemaurer left a comment

Choose a reason for hiding this comment

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

LGTM (with one formatting concern) as we're just concerned with differences from the upstream patches (most of which was smoothed over by the other PR).

@lukemaurer lukemaurer merged commit 87a4cec into oxcaml:main Jun 24, 2025
31 of 32 checks passed
mshinwell pushed a commit that referenced this pull request Jun 25, 2025
@voodoos @mshinwell
Downstreams for renaming (#3828)
70640f7 
* Merge pull request #13286 from voodoos/distinct-uids-for-interfaces

[tooling] Distinct uids for interfaces

* Merge pull request #13308 from voodoos/link-declarations

[tooling] Remember linked declarations

* Store declaration dependencies in CMS files

* Backport directionality fix from upstream ocaml/ocaml#13956

* Undo format change

(cherry picked from commit 87a4cec)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukemaurer lukemaurer lukemaurer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@voodoos @mshinwell @lukemaurer
0