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

Remove Base.getproperty #77

Closed
inkydragon opened this issue Oct 23, 2021 · 2 comments · Fixed by #99
Closed

Remove Base.getproperty #77

inkydragon opened this issue Oct 23, 2021 · 2 comments · Fixed by #99

Comments

@inkydragon
Copy link
Collaborator

After removing Base.getproperty, you need to check if the exported API is type stable and how the performance changes.

SHA.jl/src/types.jl

Lines 42 to 54 in 918aff1

function Base.getproperty(ctx::SHA2_CTX, fieldname::Symbol)
if fieldname === :state
return getfield(ctx, :state)::Union{Vector{UInt32},Vector{UInt64}}
elseif fieldname === :bytecount
return getfield(ctx, :bytecount)::Union{UInt64,UInt128}
elseif fieldname === :buffer
return getfield(ctx, :buffer)::Vector{UInt8}
elseif fieldname === :W
return getfield(ctx, :W)::Vector{UInt32}
else
error("SHA2_CTX has no field ", fieldname)
end
end

SHA.jl/src/types.jl

Lines 90 to 102 in 918aff1

function Base.getproperty(ctx::SHA3_CTX, fieldname::Symbol)
if fieldname === :state
return getfield(ctx, :state)::Vector{UInt64}
elseif fieldname === :bytecount
return getfield(ctx, :bytecount)::UInt128
elseif fieldname === :buffer
return getfield(ctx, :buffer)::Vector{UInt8}
elseif fieldname === :bc
return getfield(ctx, :bc)::Vector{UInt64}
else
error("type ", typeof(ctx), " has no field ", fieldname)
end
end

@nandoconde
Copy link

@staticfloat I think it is safe to close this based on the linked PR :D

@inkydragon
Copy link
Collaborator Author

it is safe to close this

Nope, Base.getproperty is still in the code base.

We may need to check the type stability and performance of each API after removing getproperty.
I will open a new issue to track this. (77)

This issue is opened after pr 75.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants