Skip to content

2.3 string passthrough#8

Merged
Jebbs merged 1 commit intoJebbs:2.3from
aubade:2.3-stringpassthrough
May 24, 2016
Merged

2.3 string passthrough#8
Jebbs merged 1 commit intoJebbs:2.3from
aubade:2.3-stringpassthrough

Conversation

@aubade
Copy link

@aubade aubade commented May 6, 2016

Here's the DSFMLC side of the patch that gets rid of allocations when passing string data from the D-side. I'm not incredibly familiar with C++, so I hope just having the std::string() constructors in the call statements isn't bad behavior.

The converse--minimizing or eliminating allocations when passing strings from C++ to D--is beyond my knowledge of C++ and D's underlying memory details; We might be able to do something once D's C++ interop is good enough to handle std::strings.

I've also made a minor change to the way Joystick identification is passed to avoid extraneous allocations D-side.

c.f. Jebbs/DSFML#225

return soundBuffer->This.getSamples();
}

size_t sfSoundBuffer_getSampleCount(const sfSoundBuffer* soundBuffer)
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for this change? We might need a cast on the return statement, but I think it should still return a size_t.

sfSoundBuffer_getSampleCount is defined as returning a size_t on the D side.

Copy link
Author

Choose a reason for hiding this comment

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

I made the change here because sf::SoundBuffer::getSampleCount returns a Uint64, not size_t; I forgot to make the change D-side though! I'll fix that now.

c.f. http://www.sfml-dev.org/documentation/2.3.2/classsf_1_1SoundBuffer.php#a2a791e7304553fa96269cc355cc4f7e8

Copy link
Owner

Choose a reason for hiding this comment

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

I actually have it as size_t on purpose as I personally think size_t is better to use than ulong. I have been meaning to bring this up on the forums for a long time since I think it is a better design. We have to cast it somewhere anyway since we need it to be a size_t when we make a slice. I figured it might as well be here.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair. I'll revert the change on my side, then

@Jebbs
Copy link
Owner

Jebbs commented May 23, 2016

Can you squash this down to a single commit?

@aubade aubade force-pushed the 2.3-stringpassthrough branch from 11c11d3 to 680cbe3 Compare May 23, 2016 22:39
@aubade
Copy link
Author

aubade commented May 23, 2016

Alright! I think that did it (I've never squished a git before, so I had to look up how to do it.)

@Jebbs
Copy link
Owner

Jebbs commented May 23, 2016

Looks good! 👍

I'll let it build just in case, but that was exactly what I wanted.

@aubade
Copy link
Author

aubade commented May 24, 2016

Alrighty! Looks like the appveyor results are in! Soon as you pull, I'll push the button on the D-side.

@Jebbs Jebbs merged commit 8c6c0af into Jebbs:2.3 May 24, 2016
@Jebbs
Copy link
Owner

Jebbs commented May 24, 2016

Go for it!

@aubade aubade deleted the 2.3-stringpassthrough branch May 24, 2016 03:50
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.

2 participants