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

Refactors #8

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Benjamin-McRae-Tracsis
Copy link
Contributor

No description provided.

@TeofilC
Copy link
Collaborator

TeofilC commented Jul 15, 2024

What is the bug you are trying to fix? Could you make an issue? And I'll try to help if I can.

I'm not sure about some of these refractors. Lets discuss before merging

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

RRBVector isn't being assessed as deepstrict correctly. We can fake this working by just adding it to the golden list and marking its parameter as Strict. The reason why it isn't being assessed correctly is because SmallArray# doesn't have any constructors, and thus is assumed to be deepstrict due to mconcat below.

    consDeepStrict <- traverse isConDeepStrict (TH.datatypeCons updatedDt)
    pure $ mconcat consDeepStrict

The refactors were so that the code is more easily understandable for me. I also think they make it clearer, since args is used effectively as a reader parameter but has to manually be passed around.

@TeofilC
Copy link
Collaborator

TeofilC commented Jul 16, 2024

Ah I see! You are right that's a bug.

I think only SmallArray# and Array# are susceptible to this. We need to have a special case for this somewhere. I think the easiest thing to do is to add them to the default contextOverride list. Could you make a separate MR with that change?

I'll take a proper look at this one later

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

Can do.

I fear that SmallArray# and Array# are both lazy too; would you agree with this?

@TeofilC
Copy link
Collaborator

TeofilC commented Jul 16, 2024

I fear that SmallArray# and Array# are both lazy too

Yes that's right. For any lifted type both can have thunks, for an unlifted type they are ok. So it might be good to check that eg SmallArray# Int# comes out as deep strict but SmallArray# Int doesn't.

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

I don't think there's a way currently to say "If unlifted, deepstrict; else, lazy". would have to add to the Strictness data structure

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

SmallMutableArray# will probably have the same issue, right?

@TeofilC
Copy link
Collaborator

TeofilC commented Jul 16, 2024

SmallMutableArray# will probably have the same issue, right?

Yeah mutable variants will be the same.

I don't think there's a way currently to say "If unlifted, deepstrict; else, lazy". would have to add to the Strictness data structure

Yes you'll have to add an Unlifted constructor, which means that the type has to be unlifted

@Benjamin-McRae-Tracsis
Copy link
Contributor Author

(WeakLazy, DeepStrict, Unlifted) -> pure DeepStrict

If a field's type is Unlifted, then surely it is deepstrict already?

@TeofilC
Copy link
Collaborator

TeofilC commented Jul 16, 2024

If a field's type is Unlifted, then surely it is deepstrict already?

Being unlifted is a top-level thing (https://ghc.gitlab.haskell.org/ghc/doc/users_guide/exts/primitives.html#unlifted-datatypes), eg,

data UBox a :: UnliftedType where
  UBox :: a -> UBox a

x = UBox undefined
-- while the following is not allowed
-- y :: UBox Int
-- y = undefined

Something being unlifted means that it cannot be a thunk, but it can still contain fields that can be thunks.
In fact Array# is an example of this

@Benjamin-McRae-Tracsis Benjamin-McRae-Tracsis changed the title Rrbvector fix Refactors Jul 16, 2024
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.

2 participants