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.

Sign up for GitHub

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

Jump to bottom

Fixes #4060 #4068

Merged
merged 7 commits into from Mar 1, 2017
Merged

Fixes #4060 #4068

merged 7 commits into from Mar 1, 2017

Conversation

crisward
Copy link
Contributor

Extra byte evaluated to truthy with an empty slice. Added size check which fixes the gzip bug.
I'll remove my previous pull request.

@crisward
Fixes #4060
fbbb28d 
Extra byte evaluated to truthy with an empty slice. Added size check which fixes the gzip bug.
I'll remove my previous pull request.
@crisward
slightly neater
3bf43a2
RX14
RX14 requested changes Feb 23, 2017
View reviewed changes
src/gzip/header.cr Outdated
@@ -71,7 +73,7 @@ class Gzip::Header

# flg
flg = Flg::None
flg |= Flg::EXTRA if @extra
flg |= Flg::EXTRA if !@extra.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

Use unless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have done...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

src/gzip/header.cr Outdated
@@ -85,8 +87,8 @@ class Gzip::Header
# os
io.write_byte os

if extra = @extra
io.write_byte extra.size.to_u8
if !@extra.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

@ysbaddaden
Copy link
Contributor

Thanks, I was about to suggest the same (or if @extra.any? to be positive)

@asterite
Copy link
Member

asterite commented Feb 23, 2017 •
edited

No, this fix is a workaround, the real issue is a compiler bug. @extra is always initialized, it has an initializer

@asterite asterite closed this Feb 23, 2017
asterite
asterite reviewed Feb 23, 2017
View reviewed changes
src/gzip/header.cr Outdated
@@ -44,6 +44,8 @@ class Gzip::Header
xlen = io.read_byte.not_nil!
@extra = Bytes.new(xlen)
io.read_fully(@extra)
else
@extra = Bytes.empty
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed, @extra should always be initialized. Right now it doesn't work because of #3988

@asterite asterite reopened this Feb 23, 2017
@asterite
Copy link
Member

Sorry, I closed it too soon. The fix is OK, the but adding an else; @extra = Bytes.empty should be removed from this PR.

@crisward
Copy link
Contributor Author

Ok will update.

@RX14
Copy link
Contributor

RX14 commented Feb 23, 2017

Oh, actually 👍 to if @extra.any?.

@bcardiff bcardiff added this to the Next milestone Feb 23, 2017
@asterite
Copy link
Member

Enumerable#any? determines if any of the elements is truthy. That's definitely slower and more complex than checking for empty?. Please change it :-)

@crisward
Copy link
Contributor Author

Changed it back to empty - this only runs once per gzip operation... doubt the speed difference would be significant compared to everything else in the zipping process, but I was happy with if !@extra.empty? so what do I know?

@asterite
Copy link
Member

@crisward Ah, my comment is misleading indeed, sorry. The point is that any? means "is any element in this byte slice truthy?" which is not exactly what you want here. Here we just need to know if the byte slice is empty. Using any? makes the semantic unclear for a reader that is not familiar with the code.

@crisward
Copy link
Contributor Author

Ok, new to ruby/crystal so didn't know what any meant, assumed with was the opposite to empty. Originally had it as .size > 0. Also many other language don't have unless.. Ah well, hopefully all this will be squashed into a nice merge assuming the orange turns to green.

@RX14
Copy link
Contributor

RX14 commented Feb 23, 2017

Oh, sorry @asterite, I just assumed that any? was !empty? because of @ysbaddaden's comment.

@ysbaddaden
Copy link
Contributor

Oh, I always assumed any? was just the negation of empty?. I was wrong.

@asterite
Copy link
Member

In practice it works the same if the elements will always be truthy. The problem is that any? is very short so it's very tempting to use it instead of !empty? 😄

Sija
Sija reviewed Feb 23, 2017
View reviewed changes
src/gzip/header.cr Outdated
if extra = @extra
io.write_byte extra.size.to_u8
unless @extra.empty?
io.write_byte @extra.size.to_u8
io.write(extra)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this also refer to @extra for consistency?

Copy link
Contributor Author

@crisward crisward Feb 23, 2017 •
edited

Choose a reason for hiding this comment

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

Think you're right. It used to copy the value, I'm guessing there isn't a test case for gzip with extra set. I'll update.

@ukd1
Copy link

ukd1 commented Feb 23, 2017 •
edited

This sounds like it may also fix #4064

@spalladino
Copy link
Contributor

Related to #4084 as well?

@asterite asterite merged commit 2e61cca into crystal-lang:master Mar 1, 2017
@asterite asterite mentioned this pull request Mar 1, 2017
Closed
@spalladino spalladino mentioned this pull request Mar 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@asterite asterite asterite left review comments

@RX14 RX14 RX14 requested changes

Assignees
No one assigned
Labels
kind:bug topic:stdlib
Projects
None yet
Milestone
0.21.1
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@crisward @ysbaddaden @asterite @RX14 @ukd1 @spalladino @Sija @bcardiff