Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 13, 2020

Allows describing a GetElementPointer operation on a field of an object,
complementary to the existing ones for RefValue and RefArray.

Allows describing a GetElementPointer operation on a field of an object,
complementary to the existing ones for RefValue and RefArray.
"""
RefField <: Ref{T}
Create an object reflection adaptor that represents the properties of a specific object field.
Copy link
Member

Choose a reason for hiding this comment

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

Took me a while to parse this. Maybe

Obtain a reference to a specific object field, by creating an object reflection adaptor that wraps the parent object and the properties of a specific object field.

Copy link
Member

@c42f c42f Apr 28, 2020

Choose a reason for hiding this comment

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

Or, to focus more on the interface which people will actually use:

"""
    RefField(object, field)

Obtain a reference to an individual field of an `object`, where the given `field`
may be an index or a symbol (see also [`Base.fieldindex`](@ref)).

The returned `RefField <: Ref` is an object reflection adaptor that wraps the
parent `object` and the properties of a specific object field.
"""

d::Integer
e::Complex{BigFloat}
end
@testset "RefField" begin
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@testset "RefField" begin
@testset "RefField struct" begin

@test Base.unsafe_convert(Ptr{Integer}, d) == unsafe_load(Ptr{Ptr{Integer}}(d_ptr))
@test Base.unsafe_convert(Ptr{Complex{BigFloat}}, e) == unsafe_load(Ptr{Ptr{Integer}}(e_ptr))
end
@testset "RefField" begin
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@testset "RefField" begin
@testset "RefField mutable struct" begin

b_ptr = Base.unsafe_convert(Ptr{ManyFields}, b.x) + 8
c_ptr = Base.unsafe_convert(Ptr{ManyFields}, c.x) + 16
d_ptr = Base.unsafe_convert(Ptr{ManyFields}, d.x) + 24
e_ptr = Base.unsafe_convert(Ptr{ManyFields}, e.x) + 32
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these offsets system dependent?

@c42f
Copy link
Member

c42f commented Apr 28, 2020

Looks very useful to me. One design question: is it possible to use this recursively to refer to nested fields? Reading the implementation I think not, but it would seem useful.

@c42f
Copy link
Member

c42f commented Apr 28, 2020

One design question: is it possible to use this recursively to refer to nested fields?

What if we made f an NTuple of Int to represent the "field path" inside a nested struct. Would that allow this to work fairly easily?

@vchuravy
Copy link
Member

What if we made f an NTuple of Int to represent the "field path" inside a nested struct. Would that allow this to work fairly easily?

No I don't think so, it might be better to allow RefField to wrap other Refs, which would allow us to chain.

@tkf
Copy link
Member

tkf commented Apr 28, 2020

it might be better to allow RefField to wrap other Refs, which would allow us to chain.

I guess it'd be possible to chain to RefArray, too? It'd be nice to be able to mutate a field in, e.g., an array of namedtuples.

@c42f
Copy link
Member

c42f commented Apr 29, 2020

it might be better to allow RefField to wrap other Refs, which would allow us to chain.

Yes I agree wrapping would be cleaner (especially considering interop with ArrayRef). But I wasn't sure it would work for nested immutables... there's a boundary somewhere in a chain of Refs at the innermost mutable, after which wrapping seems difficult?

Comment on lines +150 to +153
struct RefField{f,T,S} <: Ref{T}
x::RefValue{S}
RefField{f,T,S}(x::S) where {f,T,S} = new{f::Int,T,S}(RefValue(x))
end
Copy link
Member

Choose a reason for hiding this comment

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

How does this work with immutables? For example, if I have:

x = (a=1, b=2)
ra = RefField(x, :a)
rb = RefField(x, :b)
ra[] = 10
rb[] = 20

After this code, I suppose x === (a=1, b=2) still holds? How do I get (a=10, b=20)? Is there going to be some compiler intervention?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this similar to C's address operator &x.a?

@tkf
Copy link
Member

tkf commented Apr 29, 2020

FYI here is my implementation of nestable Ref (for simple isbits case): https://discourse.julialang.org/t/alternative-to-mutable-named-tuple/38375/15

@vtjnash vtjnash added the triage This should be discussed on a triage call label Apr 30, 2020
@vtjnash
Copy link
Member Author

vtjnash commented Apr 30, 2020

Yeah, that code's kinda fundamentally flawed. I do need to look closer into how Ref ends up dealing with nesting though, and probably figure out how it should work before merging this.

@JeffBezanson
Copy link
Member

Supporting nesting seems pretty complex and can maybe be deferred; for example RefArray doesn't handle nested arrays either.

@yuyichao
Copy link
Contributor

yuyichao commented Apr 30, 2020

for example RefArray doesn't handle nested arrays either.

Arrays don't have inline storage?

@JeffBezanson
Copy link
Member

Arrays don't have inline storage?

Some types of arrays do.

@ihnorton
Copy link
Member

This would be really handy! (would happily replace my poor man's addressof macro using pointer_from_objref and fieldoffset 😄)

@vtjnash
Copy link
Member Author

vtjnash commented Aug 28, 2021

yeah, I just stopped having a use case for this myself, and felt that it potentially also would ideally attempt to deal with nested pointers, for the full "mutating immutables" deal, but that is a hard problem.

@StefanKarpinski
Copy link
Member

Notes from triage: I'm personally in favor of having some version of this to round our our RefBlah APIs, although I'm not sure about this particular implementation. I don't think this should be mixed up in "mutating immutables" stuff and since it's no longer being considered for atomics, it's freed of that consideration, and can just be what it is: a way to pass around a reference to a field. If you try to mutate a field of an immutable through this, it should just fail.

@vtjnash vtjnash closed this May 25, 2023
@vtjnash vtjnash deleted the jn/reffield branch May 25, 2023 15:55
@vtjnash vtjnash restored the jn/reffield branch May 25, 2023 15:55
@giordano giordano deleted the jn/reffield branch May 25, 2023 15:57
@giordano giordano restored the jn/reffield branch May 25, 2023 15:58
@brenhinkeller brenhinkeller removed the triage This should be discussed on a triage call label Aug 8, 2023
@giordano giordano deleted the jn/reffield branch February 25, 2024 21:39
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.