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

Use more chars instead of strings #6237

Merged
merged 2 commits into from Apr 5, 2019

Conversation

wooster0
Copy link
Contributor

And it improves things like gsub(' ', "").

@jkthorne
Copy link
Contributor

jkthorne commented Jul 4, 2018

where you able to measure any performance increase or is this just to make intent clearer?

@Sija
Copy link
Contributor

Sija commented Jul 4, 2018

@wontruefree See benchmark in #5882

@wooster0
Copy link
Contributor Author

Can we maybe merge this now?

@bararchy
Copy link
Contributor

@straight-shoota, @RX14 or @sdogruyol
it seems to passed @bcardiff and seems overall like a good addition, can we have this merged?

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

It's fine by me. But I'm not a core member ;)

@wooster0
Copy link
Contributor Author

This is ready for a review I think. @jhass @mverzilli

@wooster0
Copy link
Contributor Author

wooster0 commented Sep 4, 2018

@ysbaddaden can we merge this please?

@jwoertink
Copy link
Contributor

@RX14 This might be a good one to add in to 0.27.

@j8r
Copy link
Contributor

j8r commented Dec 20, 2018

What's blocking for the merge?

@straight-shoota
Copy link
Member

@r00ster91 Could you rebase on master to make sure it's all good?

@straight-shoota straight-shoota added this to the 0.28.0 milestone Feb 11, 2019
@wooster0
Copy link
Contributor Author

There are a bunch of GC Warning: Failed to expand heap by 16777216 bytes warnings in the test_linux32 CI fail but it seems to be unrelated.

@Sija
Copy link
Contributor

Sija commented Feb 12, 2019

Ouch, CI is going rogue again :/

Multiple:

  • GC Warning: Finalization cycle involving 0xf9ff990
  • GC Warning: Failed to expand heap by 16777216 bytes

Then:

Unhandled exception in spawn: Error writing to socket: Broken pipe (Errno)
  from src/socket.cr:77:13 in 'unbuffered_write'
  from src/io/buffered.cr:123:14 in 'write'
  from src/io.cr:853:5 in 'write_byte'
  from src/io/buffered.cr:156:14 in 'write_byte'
  from src/char.cr:767:9 in 'to_s'
  from src/io.cr:184:5 in '<<'
  from src/http/client/response.cr:63:5 in 'to_io'
  from spec/std/http/client/client_spec.cr:13:7 in '->'
  from src/fiber.cr:255:3 in 'run'
  from src/fiber.cr:72:34 in '->'
  from ???

Aaand a timeout, triple strike!


@r00ster91 Did you rebase on current master?

@wooster0
Copy link
Contributor Author

@Sija Yes, I did. I did exactly this on the branch:

git fetch origin
git rebase origin/master

@asterite
Copy link
Member

Suspicious... I'm not sure it's rebased against master. Did you git pull in master first?

@j8r
Copy link
Contributor

j8r commented Feb 12, 2019

It's isn't rebased. Don't forget to git checkout master; https://github.com/crystal-lang/crystal then go to your branch and rebase, then git push -f

@straight-shoota
Copy link
Member

No, this branch is based on c163a85 which is 4 days old and doesn't include #7397.

@wooster0
Copy link
Contributor Author

wooster0 commented Feb 12, 2019

Oh, no I didn't do git pull before pushing. I just checked out this branch, fetched and rebased and then force-pushed it here. I'm pretty sure it is rebased. I've also updated my fork (4 days ago).

@Sija
Copy link
Contributor

Sija commented Feb 12, 2019

AFAIK git pull shouldn't be needed here...

@bcardiff bcardiff merged commit 359f57c into crystal-lang:master Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants