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

Renaming Ref #18990

Closed
pabloferz opened this issue Oct 17, 2016 · 22 comments
Closed

Renaming Ref #18990

pabloferz opened this issue Oct 17, 2016 · 22 comments
Labels
breaking This change will break code
Milestone

Comments

@pabloferz
Copy link
Contributor

As pointed out here #11430 (comment), here #11430 (comment) and in some of the comments on that issue, the naming convention for Ref and RefValue is probably not the best and causes some confusion.

I'd like to propose the following changes

  • Ref -> CRef, ARef (A for Abstract) or ByRef (proposed by @JeffBezanson)
  • RefValue -> Ref (and export this)

and leaving RefArray as it is.

As pointed out by @vtjnash, most of the time what people need is the abstract version, but for #11430 and #18965 having Ref to mean RefValue would be more convenient.

Also, quoting @StefanKarpinski

I think that simple concrete names should be used for simple concrete things; programming correctly to an abstraction requires awareness and care, so having a name that reminds you of that is a good thing.

@vtjnash vtjnash added needs decision A decision on this change is needed breaking This change will break code labels Oct 17, 2016
@vtjnash
Copy link
Member

vtjnash commented Oct 17, 2016

I don't agree with those comments in either of those issues. Both of them seem to be trying to code against RefValue, but should actually be written against Ref. That is also precisely why RefValue is not exported.

I can see that it is possible we should have two levels of Ref describing whether something is a c-compatible reference (ptr-convertable) or a heap-allocated/gc-managed reference. This could be CRef and Ref, where RefValue{T} <: Ref{T} <: CRef{T}, but that would require changing the meaning of Ref. Or we could choose another similar name that describes the additional abstract behavior of RefValue (that of its reference being heap allocated by Julia's GC) such as HeapRef or GCRef (where RefValue{T} <: GCRef{T} <: Ref{T}).

@yuyichao
Copy link
Contributor

The abstract type should be used in case where all these types are designed for. There s also the different use case for a mutable slot (I.e. a cheaper zero dimensional array) and it just happens that refvalue is the one that satisfies the requirements.

@pabloferz
Copy link
Contributor Author

pabloferz commented Oct 17, 2016

There s also the different use case for a mutable slot (I.e. a cheaper zero dimensional array) and it just happens that refvalue is the one that satisfies the requirements.

That's probably where the tension comes from. The need of that behavior in some places, the fact that there's already a type that behaves that way (but it's embedded into a type hierarchy envisioned for other uses) and the discomfort of duplicating code / defining a new type gives rise to these conflicting opinions. So a possible solution might need flexibility on one of these two fronts.

Irrespectively of that, I believe that the idea of introducing another level to distinguish c-compatible and gc-managed references is a good idea anyway. I like the RefValue{T} <: Ref{T} <: CRef{T} proposal better.

@pabloferz
Copy link
Contributor Author

Another option is to allow uses of RefValue as a mutable slot and properly document when people can use it and leave clear when they should use the current Ref. Granted, that wouldn't prevent misuses. That can't be prevented in general and in occasions people need to be told or referred to the manual so that is clear anyway.

@TotalVerb
Copy link
Contributor

@yuyichao If Ptr <: Ref, shouldn't the documentation be fixed to note that?

  Ref{T}

  An object that safely references data of type T. This type is guaranteed to
  point to valid, Julia-allocated memory of the correct type. The underlying
  data is protected from freeing by the garbage collector as long as the Ref
  itself is referenced.

  When passed as a ccall argument (either as a Ptr or Ref type), a Ref object
  will be converted to a native pointer to the data it references.

  There is no invalid (NULL) Ref.

There is a NULL pointer, and Ptr isn't guaranteed to point to valid Julia-allocated memory. If Ptrs are Refs, then this documentation isn't correct.

@JeffBezanson
Copy link
Member

Another possibility: rename RefValue to RefCell or Cell and export it.

@Keno
Copy link
Member

Keno commented Jul 13, 2017

To summarize discussion:

  • Get rid of Ref in ccall, use ptr instead
  • Use Any for passing mutable structs to C, this will require type asserts in C function
  • Rename Ref and RefValue
  • Stop using Ref (the abstract type) as the constructor for a one element cell

@yuyichao
Copy link
Contributor

Get rid of Ref in ccall, use ptr instead

cfunction?

@Keno
Copy link
Member

Keno commented Jul 13, 2017

Yes, both.

@yuyichao
Copy link
Contributor

So how do you express

cfunction(identity, Int, Tuple{Ref{Int}})?

@Keno
Copy link
Member

Keno commented Jul 13, 2017

cfunction(unsafe_load, Int, Tuple{Ptr{Int}}) ;)

@yuyichao
Copy link
Contributor

That becomes quite annoy. Also the case for return type.....

@Keno
Copy link
Member

Keno commented Jul 13, 2017

Using Ref as an implicit unsafe_load is a pretty rare thing to do though. Don't think I've personally ever used it that way.

@yuyichao
Copy link
Contributor

Explictly as implicit unsafe_load not really (a lot in testing code...), as general references to julia objects, a lot in callbacks JuliaMath/Cubature.jl#23 .

@yuyichao
Copy link
Contributor

Note that the code linked works for both mutable and immutable objects.

@ViralBShah
Copy link
Member

Would be good to have an owner for this one if it is to make feature freeze?

@ViralBShah ViralBShah added the triage This should be discussed on a triage call label Nov 19, 2017
@timholy
Copy link
Member

timholy commented Nov 19, 2017

Do we have a timeline for feature freeze?

@rfourquet
Copy link
Member

Do we have a timeline for feature freeze?

It was said on slack that it would be in a bit less than 3 weeks.

@timholy
Copy link
Member

timholy commented Nov 19, 2017

This is a good thing to announce on discourse. Some of us don't use slack.

@rfourquet
Copy link
Member

@KristofferC just suggested there the same a couple of hours ago!

@ViralBShah
Copy link
Member

I think a final call has yet to be taken as some folks think it isn't enough time.

@StefanKarpinski StefanKarpinski removed needs decision A decision on this change is needed triage This should be discussed on a triage call labels Nov 20, 2017
@StefanKarpinski
Copy link
Member

This doesn't seem important enough to bother with. The current system is a little confusing, but it works and no one has seen fit to fix it in a year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

No branches or pull requests

10 participants