Skip to content

Conversation

klao
Copy link
Contributor

@klao klao commented Jun 25, 2014

This is useful to access the underlying ByteArray, eg. to pass it to FFI functions.

- Document briefly Vector's and MVector's fields
@gregorycollins
Copy link
Member

I know we export it elsewhere, but personally I'm not a fan of exporting datatype constructors (it makes it difficult to change the representation later), and would prefer a function

    unsafeToByteArray :: Vector a -> ByteArray#

On the other hand, we've already exported the constructor for MVector so maybe the cat is already out of the bag.

@klao
Copy link
Contributor Author

klao commented Jun 26, 2014

Sound reasonable. But, then it would make sense to hide the MVector constructor too (otherwise you can't really change the internal representation, and just for consistency), and this would be a backward incompatible change in itself.

Anyway, whichever solution is preferred! I am OK with either.

@gregorycollins
Copy link
Member

I would say "put it to a vote" but breaking backwards compatibility would make a mess here, and (correct me if I'm wrong) I don't think we can attach a deprecation warning to the exported constructor without marking the whole type as deprecated.

I think really this means that the "exposed constructors" camp has already won here and that we should merge your change just for consistency.

gregorycollins added a commit that referenced this pull request Jun 26, 2014
Export Primitive Vector's constructor
@gregorycollins gregorycollins merged commit 5a6541a into haskell:master Jun 26, 2014
@klao klao deleted the primitive_constr branch June 26, 2014 14:46
@klao
Copy link
Contributor Author

klao commented Jun 26, 2014

Thanks!

@tibbe
Copy link
Member

tibbe commented Jun 26, 2014

I think we should expose these constructors from an .Internal module, like we do elsewhere. We have to keep the MVector(..) export in Primitive though, as I don't want to break user code.

Otherwise I think this is a good change.

@gregorycollins
Copy link
Member

Consistency is the bugbear of small minds/etc., but are you really saying you don't want them to match?

@tibbe
Copy link
Member

tibbe commented Jun 26, 2014

Only foolish consistency. The upside of having an .Internal module is the whole Primitive module can stay portable. We're working on a layer on top of ByteArray# for base that does the same.

I want both MVector(..) and Vector(..) to be exported from an .Internal module. I suggest we do nothing about the current MVector(..) export.

@hvr
Copy link
Member

hvr commented Jun 26, 2014

I'd like to at least know what the actual cost would be of not keeping the MVector(..) export. I.e., how long has the MVector(..) export been available? Do we know how many packages actually make use of that?

@gregorycollins
Copy link
Member

Do we know how many packages actually make use of that?

Impossible to know, not all the world is Hackage. That code has had a public release, we can't walk the decision back without warning end users.

@hvr
Copy link
Member

hvr commented Jun 26, 2014

would the deprecation pragma work here, btw?

@klao
Copy link
Contributor Author

klao commented Jun 26, 2014

@hvr: no, because it would also deprecate the MVector type. And also, we don't want to deprecate the value, just want to state that it should be imported from another module.

@tibbe: How about this; we add two brand new modules: Data.Vector.Primitive.Internal and Data.Vector.Primitive.Mutable.Internal, each exporting the corresponding constructor, and add a note in MVector's doc that it should be imported from the internal module. Is this your suggestion?

@tibbe
Copy link
Member

tibbe commented Jun 26, 2014

Yes.

On Thu, Jun 26, 2014 at 9:08 PM, Mihaly Barasz notifications@github.com
wrote:

@hvr https://github.com/hvr: no, because it would also deprecate the
MVector type. And also, we don't want to deprecate the value, just want to
state that it should be imported from another module.

@tibbe https://github.com/tibbe: How about this; we add two brand new
modules: Data.Vector.Primitive.Internal and
Data.Vector.Primitive.Mutable.Internal, each exporting the corresponding
constructor, and add a note in MVector's doc that it should be imported
from the internal module. Is this your suggestion?


Reply to this email directly or view it on GitHub
#31 (comment).

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.

4 participants