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

Avoid rebuilding cmi_info record when creating .cmti files #781

Merged
merged 1 commit into from Aug 30, 2016

Conversation

Projects
None yet
2 participants
@alainfrisch
Contributor

alainfrisch commented Aug 26, 2016

Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function. In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  • Duplicated entry in cmi_crcs for the current unit as noted in #744.
  • Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 26, 2016

Contributor

@lefessan : as the original author for this piece of code, do you agree with the change?

Contributor

alainfrisch commented Aug 26, 2016

@lefessan : as the original author for this piece of code, do you agree with the change?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Does anyone want to review this?

Contributor

alainfrisch commented Aug 30, 2016

Does anyone want to review this?

@@ -91,8 +91,7 @@ val save_cmt :
binary_annots ->
string option -> (* source file *)
Env.t -> (* initial env *)
Types.signature option -> (* if a .cmi was generated,
the signature saved there *)
Cmi_format.cmi_infos option -> (* if a .cmi was generated *)

This comment has been minimized.

@diml

diml Aug 30, 2016

Member

The doc comment of the function should probably be changed to say cmi instead of sg. Also, this is not related to this patch but the order of argument names looks wrong

@diml

diml Aug 30, 2016

Member

The doc comment of the function should probably be changed to say cmi instead of sg. Also, this is not related to this patch but the order of argument names looks wrong

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Thanks! Fixed.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Thanks! Fixed.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Aug 30, 2016

Member

I read the patch, it looks correct to me. I was just not sure about setting the cmt_imports field to Env.imports () when a cmi is provided, but at the same time save_cmt is already reading several global states

Member

diml commented Aug 30, 2016

I read the patch, it looks correct to me. I was just not sure about setting the cmt_imports field to Env.imports () when a cmi is provided, but at the same time save_cmt is already reading several global states

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

I was just not sure about setting the cmt_imports field to Env.imports () when a cmi is provided

Note that this PR does not change this behavior. One could perhaps use the list of imports from the .cmi instead, but this would make the code less regular (one would still need to call Env.imports when the cmi is not provided).

Contributor

alainfrisch commented Aug 30, 2016

I was just not sure about setting the cmt_imports field to Env.imports () when a cmi is provided

Note that this PR does not change this behavior. One could perhaps use the list of imports from the .cmi instead, but this would make the code less regular (one would still need to call Env.imports when the cmi is not provided).

Avoid rebuilding cmi_info record when creating .cmti files
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.

@alainfrisch alainfrisch merged commit 227bdc6 into ocaml:trunk Aug 30, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Avoid rebuilding cmi_info record when creating .cmti files (#781)
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment