-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update locations during destructive substitutions #10405
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,13 +31,15 @@ type t = | |
modules: Path.t Path.Map.t; | ||
modtypes: module_type Path.Map.t; | ||
for_saving: bool; | ||
loc: Location.t option; | ||
} | ||
|
||
let identity = | ||
{ types = Path.Map.empty; | ||
modules = Path.Map.empty; | ||
modtypes = Path.Map.empty; | ||
for_saving = false; | ||
loc = None; | ||
} | ||
|
||
let add_type_path id p s = { s with types = Path.Map.add id (Path p) s.types } | ||
|
@@ -54,8 +56,13 @@ let add_modtype id ty s = add_modtype_path (Pident id) ty s | |
|
||
let for_saving s = { s with for_saving = true } | ||
|
||
let change_locs s loc = { s with loc = Some loc } | ||
|
||
let loc s x = | ||
if s.for_saving && not !Clflags.keep_locs then Location.none else x | ||
match s.loc with | ||
| Some l -> l | ||
| None -> | ||
if s.for_saving && not !Clflags.keep_locs then Location.none else x | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right ordering of tests? If I understand correctly, the idea of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These features do not interact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the two orderings are equivalent, I would maybe have a preference for the one that lets me easily reason about the fact that I will get |
||
|
||
let remove_loc = | ||
let open Ast_mapper in | ||
|
@@ -547,6 +554,11 @@ and modtype_declaration scoping s decl = | |
let merge_path_maps f m1 m2 = | ||
Path.Map.fold (fun k d accu -> Path.Map.add k (f d) accu) m1 m2 | ||
|
||
let keep_latest_loc l1 l2 = | ||
match l2 with | ||
| None -> l1 | ||
| Some _ -> l2 | ||
|
||
let type_replacement s = function | ||
| Path p -> Path (type_path s p) | ||
| Type_function { params; body } -> | ||
|
@@ -563,4 +575,5 @@ let compose s1 s2 = | |
modules = merge_path_maps (module_path s2) s1.modules s2.modules; | ||
modtypes = merge_path_maps (modtype Keep s2) s1.modtypes s2.modtypes; | ||
for_saving = s1.for_saving || s2.for_saving; | ||
loc = keep_latest_loc s1.loc s2.loc; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why
change_locs
instead ofchange_loc
? (As far as I can tell there is only one location.)Also: as far as I can tell this function is only ever called on
Subst.identity
which hasloc = None
; could it maybe be made slightly more principled by becomingwith_loc
, that fails if the subtitution already has a location set?(If I understand correctly, this
loc
parameter could have been made a parameter ofidentity
, but that would make the patch much more invasive -- I count 57 uses of Subst.identity. Or is there an important reason to specify it separately?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but several items (usually)
Sounds great.
No.