Make T.instance inferable #30086
Make T.instance inferable #30086
Conversation
vtjnash
left a comment
vtjnash
left a comment
There was a problem hiding this comment.
I don't know—this seems like an implementation detail that we don't want people to depend upon
base/compiler/tfuncs.jl
Outdated
| return Const(isdefined((arg1::Const).val, idx)) | ||
| elseif isa(arg1, Const) | ||
| arg1v = (arg1::Const).val | ||
| if isimmutable(arg1v) || (isa(arg1v, DataType) && is_dt_const_field(idx)) |
There was a problem hiding this comment.
could this be:
if isimmutable(arg1v) || isdefined(arg1v, idx)There was a problem hiding this comment.
Well, not quite, because you do want false here if .instance is not defined. However, since it shouldn't be possible to undefine a field, I think isimmutable(arg1v) || isdefined(arg1v, idx) || (isa(arg1v, DataType) && is_dt_const_field(idx)) would be ok.
base/compiler/tfuncs.jl
Outdated
| end | ||
| add_tfunc(<:, 2, 2, subtype_tfunc, 0) | ||
|
|
||
| is_dt_const_field(fld) = ( |
There was a problem hiding this comment.
or rather, I'm missing ::Int.
There was a problem hiding this comment.
Ah, yes. Last time I looked, I think we were missing that check in the caller, but looks like we now ensure it is an Int.
Fair, but is that a reason to intentionally make it slower? |
Perhaps? It makes some of the code more complex, and as they say "writing code is writing bugs". Also perhaps not as useful to optimize a code smell pattern that we might want to explicitly discourage (since only a subset of objects have this field defined, and the user should be tracking values not Types). But anyways, I won't block the PR over it. |
No description provided.