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 non-existent property #75

Merged
merged 10 commits into from
Nov 13, 2023
Merged

Conversation

inkydragon
Copy link
Collaborator

@inkydragon inkydragon commented Oct 22, 2021

@staticfloat

tl; dr

  1. rm if branch in Base.getproperty(::SHA2_CTX, )
  2. Make function getproperty(::SHA2_CTX, ) and function getproperty(::SHA3_CTX, ) error messages consistent
  3. rm trailing spaces in test
  4. add more test for Base.getproperty

1. rm if branch

Property W only exists in SHA1_CTX, not exists in <:SHA2_CTX.

SHA.jl/src/types.jl

Lines 10 to 15 in 918aff1

mutable struct SHA1_CTX <: SHA_CTX
state::Array{UInt32,1}
bytecount::UInt64
buffer::Array{UInt8,1}
W::Array{UInt32,1}
end

SHA.jl/src/types.jl

Lines 18 to 22 in 918aff1

mutable struct SHA2_224_CTX <: SHA2_CTX
state::Array{UInt32,1}
bytecount::UInt64
buffer::Array{UInt8,1}
end

2. error msg

error("SHA2_CTX has no field ", fieldname)

error("type ", typeof(ctx), " has no field ", fieldname)

3. more test

https://app.codecov.io/gh/JuliaCrypto/SHA.jl/blob/master/src/types.jl


And furthermore, maybe we can delete the entire Base.getproperty(::Union{SHA2_CTX, SHA3_CTX}, ...).

@inkydragon
Copy link
Collaborator Author

One question: is this a bug?

state_type(::Type{SHA3_CTX}) = UInt64

-state_type(::Type{SHA3_CTX}) = UInt64
+state_type(::Type{T}) where {T<:SHA3_CTX} = UInt64

@staticfloat
Copy link
Member

This all looks good to me; and yes, I believe that SHA3_CTX should be subtyped like you suggest.

@staticfloat
Copy link
Member

Personally, I don't think the getproperty() stuff is that useful, so I'd be okay with even just removing it all. I'm not sure it's actually used anywhere. :P

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (068f85d) 98.14% compared to head (967a110) 98.96%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   98.14%   98.96%   +0.82%     
==========================================
  Files           9        9              
  Lines         484      482       -2     
==========================================
+ Hits          475      477       +2     
+ Misses          9        5       -4     
Files Coverage Δ
src/types.jl 100.00% <100.00%> (+5.00%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Test `short_blocklen` - call -> `state_type`
@inkydragon
Copy link
Collaborator Author

Ready for merge.

Personally, I don't think the getproperty() stuff is that useful, so I'd be okay with even just removing it all.

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)

@staticfloat
Copy link
Member

This appears to have slipped through the cracks, I'm sorry. Can you resolve the conflicts?

@staticfloat staticfloat merged commit 88e1c83 into JuliaCrypto:master Nov 13, 2023
11 checks passed
@staticfloat
Copy link
Member

Thanks for taking that over the finish line, @inkydragon !

@inkydragon inkydragon deleted the getproperty branch November 13, 2023 03:36
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.

3 participants