-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Rename isleaftype() to isconcrete() #20709
Conversation
To be completely explicit about it, I'd like to propose that we officially define |
Related to #17086 |
Thanks. Regarding the definition of "concrete", see in particular the table at #17086 (comment). |
@@ -264,8 +264,8 @@ First, a review of some relevant Julia type terminology: | |||
|
|||
| Syntax / Keyword | Example | Description | | |||
|:----------------------------- |:------------------------------------------- |:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | |||
| `mutable struct` | `String` | "Leaf Type" :: A group of related data that includes a type-tag, is managed by the Julia GC, and is defined by object-identity. The type parameters of a leaf type must be fully defined (no `TypeVars` are allowed) in order for the instance to be constructed. | | |||
| `abstract type` | `Any`, `AbstractArray{T, N}`, `Complex{T}` | "Super Type" :: A super-type (not a leaf-type) that cannot be instantiated, but can be used to describe a group of types. | | |||
| `mutable struct` | `String` | "Leaf Type" :: A group of related data that includes a type-tag, is managed by the Julia GC, and is defined by object-identity. The type parameters of a concrete type must be fully defined (no `TypeVars` are allowed) in order for the instance to be constructed. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I missed non-lowercase occurrences of "lift". Will fix later, after leaving time for other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Though I've noticed that table is almost impossible to read online. Any ideas how to improve it? Is it possible to force text to wrap instead of going beyond the page limits? Or should we move to a bullet list instead?
I happened to notice yesterday that the |
Looking at that table is a good way to waste time and become confused. |
Is there any practical reason to need to distinguish any other meaning of "concrete"? The meaning I've proposed maps well onto whether a field of a type can be stored inline assuming it is a bits type. When would you need any of the other meanings? In particular, the meaning by which |
I'm fine with this renaming, but note that as it is we'd have |
Under what theory of concreteness is |
base/reflection.jl
Outdated
true | ||
|
||
julia> isleaftype(Vector{Complex{Float32}}) | ||
julia> isconcrete(Vector{Complex{Float32}}) | ||
true | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closing triple backtick missing here, as mentioned by @tkoolen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See new commit.
It's concrete in the sense that you have enough information about the value to do dispatch. This is probably part of why we used the term "leaf type"; if not for that we might have called this "concrete" all along. |
But aren't |
For consistency, also change the C function jl_is_leaf_type() to jl_is_concrete_type(). Replace (almost) all uses of "leaf" which mean "concrete".
1e24b6d
to
471791d
Compare
Unfortunately it is a bit context-dependent. If you want to know the memory layout of a value, then knowing that it's a Here, we can only make sense of
We have a lot of experience with the For an example library use, consider the |
I propose having |
What is an example use case where you need to know that something could be the direct tag of an object? Or, put differently, an example of where calling |
(Cf. #20555 "A publicly facing reflection API" -> knowing which types you can dispatch on?) |
Bump. Should I rebase this so that it can be merged? |
21bab13
to
fd4283c
Compare
Rebased and added a NEWS.md entry. |
base/deprecated.jl
Outdated
@@ -1595,6 +1595,8 @@ end | |||
# issue #6466 | |||
# `write` on non-isbits arrays is deprecated in io.jl. | |||
|
|||
@deprecate isconcrete isconcrete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deprecate isleaftype isconcrete
? Or are you trying to deprecate what you just introduced?... by what you just introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still much prefer "concrete" to "leaf type" but there's a bit of a fundamental terminology decision to be made here. Does this function actually compute the property that we want to call "concrete"? |
I added the 1.0 milestone as we should probably make a final decision on this before release |
@JeffBezanson would you be happier calling this |
One good option might be to un-export |
That's a good plan. |
Done by #23666 |
For consistency, also change the C function jl_is_leaf_type() to
jl_is_concrete_type(). Replace (almost) all uses of "leaf" which mean
"concrete".
I have left unchanged several functions in
src/typemap.c
(sig_match_leaf
,sig_match_by_type_leaf
,is_cache_leaf
,is_leaf_bound
) and theleaf
field of_jl_typemap_entry_t
, since the "leaf" term seemed to make more sense there in the context of a type tree. Suggestions welcome.Fixes #17086.