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

Replace aliases in Aliases_of_canonical_element.t with 3 fields #383

Open
wants to merge 1 commit into
base: flambda2.0-stable
Choose a base branch
from

Conversation

lukemaurer
Copy link

Aliases_of_canonical_element.t carries around a map from Name_mode.t
to Simple.Set.t so that it can compute the earliest element in each
name mode. This is less efficient than simply having a field for each
name mode. Furthermore, we don't need the sets if we just track the
earliest for each name mode (along with its binding time).

`Aliases_of_canonical_element.t` carries around a map from `Name_mode.t`
to `Simple.Set.t` so that it can compute the earliest element in each
name mode. This is less efficient than simply having a field for each
name mode. Furthermore, we don't need the sets if we just track the
earliest for each name mode (along with its binding time).
@mshinwell
Copy link

@poechsel is going to review this!

@mshinwell mshinwell requested a review from poechsel March 31, 2021 14:49
update_all_earliest t ~f:(fun earliest ->
match earliest with
| Earliest { name; _ } when Simple.is_var name -> No_alias
| _ -> earliest)

Choose a reason for hiding this comment

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

Please use No_alias in that case

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, which case?

{ t with all; }

let move_variables_to_mode_in_types t =
update_all_earliest t ~f:(fun earliest ->
Copy link

Choose a reason for hiding this comment

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

This looks wrong. I would expect earliest and earliest_ge_in_types to be left unchanged by this function.

Copy link

Choose a reason for hiding this comment

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

(Although #393 gets rid of this function completely, so maybe it isn't that important anyway)

Copy link
Author

Choose a reason for hiding this comment

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

!! Yes, of course.

This was referenced Apr 1, 2021
lukemaurer added a commit that referenced this pull request Apr 30, 2021
This reverts the parts of commit
9816841 that have been split off into
PRs #383 and #386, as well as the Bottom stuff (likely to be unnecessary
as we're considering making `Coercion.compose` a total function).
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.

4 participants