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

Remove deprecated functions #10867

Merged
merged 17 commits into from
Jan 12, 2022
Merged

Remove deprecated functions #10867

merged 17 commits into from
Jan 12, 2022

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Jan 10, 2022

This PR proposes removing a bunch of deprecated functions in the 5.0 release.

@nojb nojb force-pushed the remove_deprecated branch 2 times, most recently from 26b471d to 187a426 Compare January 10, 2022 19:03
@avsm
Copy link
Member

avsm commented Jan 11, 2022

For the Obj.truncate removal, I notice that there are a few other places where Obj.truncate has hindered some optimisations in the past:

asmcomp/cmm_helpers.ml
649:  (* We cannot deem this as [Immutable] due to the presence of [Obj.truncate]
middle_end/semantics_of_primitives.ml
77:      No_effects, Has_coeffects  (* That old chestnut: [Obj.truncate]. *)

It's probably better to fix these separately from this PR, but worth a followup issue to not forget?

Copy link
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

LGTM

@nojb
Copy link
Contributor Author

nojb commented Jan 12, 2022

LGTM

Thanks, planning to merge by end of day unless someone objects.

@nojb
Copy link
Contributor Author

nojb commented Jan 12, 2022

It's probably better to fix these separately from this PR, but worth a followup issue to not forget?

Thanks @avsm, will make a new issue with your note once the PR is merged.

Copy link
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

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

I read through all these and it looks good to me as well.

@Octachron
Copy link
Member

I have some doubts about the tag-related function in Format: the semantic tags were only introduced with 4.08, they are more useful than string tags in many cases but not all cases. However, the string tags are trivially implemented with the semantic tags, thus it might be better that I write a compatibility format library for potentially remaining users of string tags.

@nojb
Copy link
Contributor Author

nojb commented Jan 12, 2022

I have some doubts about the tag-related function in Format: the semantic tags were only introduced with 4.08, they are more useful than string tags in many cases but not all cases. However, the string tags are trivially implemented with the semantic tags, thus it might be better that I write a compatibility format library for potentially remaining users of string tags.

Thanks for chiming in @Octachron! Sorry if I missed something, but are you suggesting any changes to the present PR, or was your comment just to float the idea of a compatibility library? I never use such libraries myself, but I don't have any objections to having one either :)

@Octachron
Copy link
Member

Sorry, I meant to say that the deprecated string tag functions might have still have some active users in the wild: the deprecation is relatively recent (only 3 years) and the new API can be slightly more cumbersome in very specific cases. Thus I expect that this specific change will break some existing code somewhere.

However, since those deprecated functions can be easily implemented in a library, the simplest choice seems to offer a compatibility library for people that don't or can't move to the new semantic tags.

In short, I am offering to write a compatibility library without changing anything in this PR (beyond keeping a record in this very discussion that those functions might require a compatibility library).

@nojb nojb merged commit 2526e22 into ocaml:trunk Jan 12, 2022
@gasche
Copy link
Member

gasche commented Jan 19, 2022

Note: some code breakage due to the removal of the Format string-tag API is being discussed in #10917.

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.

5 participants