Conversation
middle_end/closure/closure.ml
Outdated
| assert (Ident.is_global_or_predef id); | ||
| let pack_prefix = | ||
| if Ident.is_global id then Compilenv.pack_prefix_for_global_ident id | ||
| else Compilation_unit.Prefix.empty | ||
| in | ||
| let symbol = | ||
| Symbol.for_ident id ~pack_prefix | ||
| |> Symbol.linkage_name | ||
| in |
There was a problem hiding this comment.
This code really doesn't belong here - almost exactly the same code needs to be added to Opttoploop for the same purpose: translating a global Ident.t to a linkage name. Compilenv.symbol_for_global used to do this, and Compilenv still needs to be consulted anyway, so we should just keep Compilenv.symbol_for_global after all (with this new implementation).
There was a problem hiding this comment.
I'm not sure about this (one thing is that Flambda 1 doesn't seem to need such an interface in Compilenv). When Ident.t knows the Compilation_unit.t, Compilenv won't need to be involved, so it may be better to tolerate the code duplication in this intermediate state of the work.
There was a problem hiding this comment.
I'd still rather it be in just one place, even if we're about to get rid of it. It doesn't need to be in Compilenv necessarily.
lpw25
left a comment
There was a problem hiding this comment.
Some comments from when we looked over the code yesterday
utils/compilation_unit.mli
Outdated
|
|
||
| (** The type used for recording digests of compilation units in .cmo and | ||
| .cmx files. *) | ||
| type crcs = (t * Digest.t option) list |
There was a problem hiding this comment.
Deleted. (Once we get to .cmis, this type could replace Misc.crcs, but even then I'm not sure it's needed: are the modules in a crcs list ever not just the base names of modules in the same pack - meaning we don't need to store the prefix?)
middle_end/symbol.ml
Outdated
| let compilation_unit = CU.get_current_exn () in | ||
| let linkage_name = | ||
| linkage_name_for_compilation_unit compilation_unit | ||
| ^ separator ^ Ident.unique_name id |
There was a problem hiding this comment.
I think you need to check the sizes of artifacts to make sure that we aren't increasing their size by losing sharing on strings.
asmcomp/cmm_helpers.ml
Outdated
| Symbol.for_compilation_unit compilation_unit | ||
| |> Symbol.linkage_name | ||
| in | ||
| compilation_unit_symbol ^ Symbol.separator ^ name |
There was a problem hiding this comment.
I think this logic should be moved into Symbol with then this can be replaced by code that creates a symbol and extracts its linkage name.
There was a problem hiding this comment.
I rewrote this to leave the string manipulation to Symbol. Cmm_helpers.make_symbol is still there for the convenient optional argument and call to Symbol.linkage_name, which seems much more "helper"-like.
| val predef_exn : t | ||
|
|
||
| (** The name of the compilation unit, excluding any [for_pack_prefix]. *) | ||
| val name : t -> Name.t |
There was a problem hiding this comment.
Perhaps this should be basename by analogy to Unix. (This is particularly desirable since a Compilation_unit.t is (currently) often called a "name", and asking the basename of a name is perfectly sensible but asking the name of a name is not.)
This lets single-module executables to still have weird names like `.cinaps`.
|
Closing because this ended up as oxcaml/oxcaml#753. |
Some cleanups for
Symbol,Compilation_unit, andCompilenvon the way to the next PR, which will further streamline the handling of symbols by having each globalIdent.tpoint directly to itsCompilation_unit.t.