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

Make Ident.t abstract and immutable. #1704

Merged
merged 7 commits into from Apr 9, 2018

Conversation

Projects
None yet
4 participants
@Drup
Copy link
Contributor

Drup commented Apr 6, 2018

The initial version of this patch made Ident.t abstract to avoid exposing the mutability, but I realized that the mutability was not really used anywhere anymore (it used to be important for the toplevel), so I made it immutable as well.

This is useful for compiler forks that play tricks with Idents, in particular the one that have additional flags like cough Eliom cough and MetaOCaml.
With this, one can modify some functions inside Ident in a way that will not break with future code in other parts of the compiler.

This change is technically breaking. For example, js_of_ocaml's toplevel used to access idents's fields directly (this is not the case anymore). I'm not sure how much it is used otherwise (I guess not much).

@Drup Drup force-pushed the Drup:ident_abstr branch from 59114a8 to 6ca7e16 Apr 6, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

I made a couple of minor edits. This is a good change.

@mshinwell mshinwell merged commit b073f82 into ocaml:trunk Apr 9, 2018

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

This comment has been minimized.

Copy link
Contributor

Drup commented on middle_end/base_types/compilation_unit.ml in a831af3 Apr 9, 2018

No @@ and other |> in flambda ?

@@ -18,6 +18,7 @@

include Variable

<<<<<<< HEAD

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Left-over of a conflict marker.

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

@mshinwell : careful, I think you merged a broken commit!

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Apr 9, 2018

I've pushed a tentative fix for the build (7110e3e).

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 9, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 9, 2018

Thanks for the fixes @mshinwell , but please let me rebase before merging next time. :)

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

Sorry, I thought I had built and tested this, but was mistaken.

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 9, 2018

@mshinwell Even then, merge commits in feature branches make the commit history very hard to read and are often unnecessary. For such small patchset, rebasing is both easier and produces a clean history.
Please let me rebase my patchsets before merging them.

@Drup Drup deleted the Drup:ident_abstr branch Jan 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.