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

Scrape types in Typeopt.maybe_pointer #1722

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

lpw25 commented Apr 13, 2018

Substitutions don't update the immediacy information of types. So code like:

module type T = sig
    type underlying
    type t = private underlying
  end

  module type S = sig
    include T with type underlying = int
    val zero : t
  end

  module X : S

Has X.t marked as non-immediate, even though it is a private abbreviation for int. This means that we get a call to caml_modify for:

let f (x : X.t ref) (y : X.t) =
  x := y

This patch remedies this by fully expanding the type before checking its immediate status. We already do this when classifying the type for array operations, so it is not a surprising change.

@stedolan

This comment has been minimized.

Copy link
Contributor

stedolan commented Apr 16, 2018

I am not at all an expert on this code, so I probably misunderstand this, but from this it looks like it is never a good idea to call Ctype.maybe_pointer_type without having done Typeopt.scrape_ty first. If that is the case, would it be better to remove Ctype.maybe_pointer_type completely and inline the code into Typeopt.maybe_pointer_type ?

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Apr 16, 2018

Ctype.maybe_pointer_type is also used for the type-checking of [@@immediate] annotations. I chose not to fix that use in this PR because it affects typing and I'm not 100% sure it is the right fix there -- whereas this change is definitely an improvement and I'd like to get it in 4.07.

@trefis

trefis approved these changes Apr 20, 2018

Copy link
Contributor

trefis left a comment

LGTM.

The test failure is due to the absence of a changelog entry, I think you should add one.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Apr 20, 2018

@damiendoligez Am I alright to cherry-pick this to 4.07? It's a one line bug fix and I've seen it hit a couple of times in practice.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 20, 2018

I would believe it's ok to merge this kind of non-invasive bugfixes in 4.07 at this point in time.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Apr 23, 2018

I apparently couldn't push a changelog entry to the PR.
So I pushed this PR+change entry to trunk directly and will now cherry-pick on 4.07.

@trefis trefis closed this Apr 23, 2018

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.