search

ezrosent / allocators-rs

Allocators in Rust
Apache License 2.0
311 stars 28 forks source link

103 fix #123

Closed getynge closed 7 years ago

getynge commented 7 years ago

Simple fix to segfault when using bsalloc on windows. However on my system build fails because libc::write (used in alloc-fmt) expects a u32 as its last argument instead of a usize, however CI fails if I attempt to correct this. I would like to confirm if this is a windows-specific issue and should be addressed in its own issue.

joshlf commented 7 years ago

Travis CI is experiencing a Linux outage right now. Are you sure the Travis failure you got wasn't just due to that outage?

joshlf commented 7 years ago

Hey @getynge, thanks for the PR and welcome!

@ezrosent and I still have to figure out how we want to deal with Windows support inside the allocator - the prospect of a fix like this for bsalloc has been discussed before, and we're not sure whether we want to do this, leave it as the caller's responsibility to commit memory, or something else entirely.

My advice would be to remove the bsalloc changes from this PR and just keep the fix for #124, since the latter we can merge immediately. It's up to you, of course.

If you decide to just keep the fix so we can merge immediately, then once you've got the code how you like it, I'd ask you to squash everything into a single commit with a message something along the lines of (see the CONTRIBUTING.md section on commit messages for details):

alloc-fmt: Fix Windows compilation bug

- Closes #124
getynge commented 7 years ago

I'll go ahead and remove the fix. Sorry about that I thought I already removed it from the PR but I guess not

getynge commented 7 years ago

Alright, I'll wait for CI to complete but I tested locally and the changes worked on my end. Rolled back bsalloc segfault fix for now

joshlf commented 7 years ago

OK sounds good, thanks!

Looks like the bsalloc changes are still there?

getynge commented 7 years ago

okay looks like when I squashed the commit that last change got dropped, fixed it. sorry about that

joshlf commented 7 years ago

Looks like it's still not rolled back. Line 12 should read use super::mmap_alloc::MapAlloc;, line 25 should read ma: MapAlloc,, and line 33 should read ma: MapAlloc::default(),. If I'm reading the diff right, those three changes should be sufficient to get bsalloc.rs back to its original state.

getynge commented 7 years ago

okay did that. Sorry about all that, it's been a while since I've last contributed to a project

joshlf commented 7 years ago

Looks like you're missing a trailing ; after the use line and a trailing , after the ma: line :)

And no worries - Git is black magic. Fun fact: Git was originally designed to be the backend of a tool that itself would be human-facing and human-usable. Everything makes more sense once you learn that...

joshlf commented 7 years ago

I am so confused... it looks like your latest commit goes from having a comma to not having it, but I don't see a commit that adds the comma back... Do you want to just blow away these changes and start from scratch and add a new PR? That might be the easiest thing to do here.

getynge commented 7 years ago

Yeah that would be best, I'll just clone from master again tomorrow morning and do it again.
thanks for helping me through the first set of commits though, it's just been a while but I should have wound back up tomorrow or so.

I've been using git for five years and I still do this on new projects 👍

getynge commented 7 years ago

Closing to open new PR tomorrow