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

tweak SHA1 printing #27036

Merged
merged 8 commits into from
May 16, 2018
Merged

tweak SHA1 printing #27036

merged 8 commits into from
May 16, 2018

Conversation

simonbyrne
Copy link
Contributor

  1. Print quotes around byte string in show
  2. Add a print method to enable string interpolation

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

I approve, modulo the test failures, of course. Have intended to do this for a while now.

@StefanKarpinski
Copy link
Sponsor Member

Incidentally, we should do the same with the printing of UUIDs.

@ararslan ararslan added the display and printing Aesthetics and correctness of printed representations of objects. label May 9, 2018
@stevengj
Copy link
Member

Needs a test?

test/loading.jl Outdated
@@ -86,6 +86,18 @@ import UUIDs: UUID, uuid4, uuid_version
import Random: shuffle, randstring
using Test

shastr = "ab"^20
hash = SHA1(shastr)
@test hash == eval(sprint(io -> show(io, hash))) # check show method
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean eval(parse(string(hash)))?

test/loading.jl Outdated
@@ -88,13 +88,13 @@ using Test

shastr = "ab"^20
hash = SHA1(shastr)
@test hash == eval(sprint(io -> show(io, hash))) # check show method
@test hash == eval(parse(sprint(io -> show(io, hash)))) # check show method
Copy link
Member

Choose a reason for hiding this comment

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

Isn't string(hash) equivalent to sprint(show, hash) which is equivalent to sprint(io -> show(io, hash))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this PR was to make string(hash) and show(hash) different. I didn't think of using sprint(show, hash) though, good call.

Copy link
Member

@stevengj stevengj May 11, 2018

Choose a reason for hiding this comment

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

Couldn't you use repr(hash) then?

(Sorry, string is equivalent to print, and repr is equivalent to show.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I keep forgetting about those functions.

test/loading.jl Outdated
hash = SHA1(shastr)
@test hash == eval(parse(repr(hash))) # check show method
@test string(hash) == shastr
@test "check $hash" == "check $shastr"
Copy link
Member

Choose a reason for hiding this comment

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

The last two tests seem redundant because interpolation just calls string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not the case on current master:

julia> hash = Base.SHA1("aa"^20)
SHA1(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)

julia> "check $hash"
"check SHA1(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)"

julia> string(hash)
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

Copy link
Member

Choose a reason for hiding this comment

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

What? I just looked at @code_lowered for "foo $x", and it lowers to string("foo ", x), which calls print_to_string, which calls print (not show).

What am I missing?

Copy link
Member

Choose a reason for hiding this comment

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

The documentation of string(xs...) also says that it calls print. Something seems wrong here.

string(hash::SHA1) = bytes2hex(hash.bytes)
print(io::IO, hash::SHA1) = print(io, string(hash))
Copy link
Member

Choose a reason for hiding this comment

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

This seems suboptimal: It seems like we should have a bytes2hex(io, bytes) method that writes the hex digits to a stream rather than allocating a string. That way you can print the hex digits directly instead of allocating a string first.

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski May 16, 2018

Choose a reason for hiding this comment

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

Let's get this merged and tweak for optimality in the future; issue: #27121.

test/loading.jl Outdated
@@ -86,6 +86,18 @@ import UUIDs: UUID, uuid4, uuid_version
import Random: shuffle, randstring
using Test

shastr = "ab"^20
hash = SHA1(shastr)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

These variables should all be let-bound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants