-
Notifications
You must be signed in to change notification settings - Fork 22
2.3 string passthrough #226
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
Conversation
|
This is a lot of stuff to go through and I'm pretty swamped right now. Hopefully you don't feel too bad letting this sit here for a week or so while I get some things cleared of my plate. |
|
Yeah, no worries. dub makes building off my local branch pretty painless, so my own personal needs are satisfied; this is just to help fix things for everyone else! |
|
Though for anyone lurking, I'd love if people could test the 2.3 branch (or this one)! There are parts of DSFML I'm not using right now in my project (notably, the networking module and text objects), so I'd really appreciate if people could try and tear it apart. I'll see about posting some windows binaries for 2.3 DSFMLC if that would help. |
|
I'm going to try go through this and hopefully get it in sometime this week. I might have you do some squashing after that and then announce the branch on the D forums to get more people to test it out. :P |
| * Returns: True if loading succeeded, false if it failed | ||
| */ | ||
| bool loadFromFile(string filename) | ||
| bool loadFromFile(const(char)[] filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the reason for using const(char)[] instead of string? So that one could use mutable char arrays too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precisely, or to pass from another function when we don't have guaranteed-immutable data on hand.
AFAICT C++ const doesn't demand the D promise of immutability and it only needs the data for the duration of the call anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a perfect world we'd accept ranges of char for these, as that would allow us to combine paths, filenames, and extensions without causing allocation, but the OS demands a contiguous chunk of memory with the whole name in it. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we only need to avoid GC allocations. Maybe we could do some special optimizations for ranges if we wanted to add those down the line. For example, we could malloc a buffer, fill it with the range data, and we're good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible, and the new std.experimental.allocator stuff could easily do this!
On the other hand it might be better to ask the user to take that step themself instead of doing it for them. More realistically, i think this is probably the best solution until D is able to directly access std::strings (I've heard vague rumblings on the newsgroups to this effect, so I'm keeping my eyes open!)
c.f. dlang/druntime#1316
|
Here's the plan: After that is merged, update the travis file to refer back to the 2.3 branch back on https://github.com/Jebbs/DSFMLC.git and then squash this into a single commit and I will merge it next. Awesome job with this stuff! Because of all your hard work I'm aiming for a edit: Knowing me, this is more feasible. :P |
|
Fantastic; shall I squish this branch down too after I update the CI files? |
|
Yes please! |
Accept const(char)[] where feasible Cache Joystick Identification
18899e3 to
8d44557
Compare
|
This closes #225, yes? |
|
It does, indeed! |
This is the D-side of Jebbs/DSFMLC#8 to fix #225 and any related issues.
I've made a lot of the D-side functions request const(char)[] instead of string, because as far as i can tell most of these don't need to store the string data on the C++ side, making a D-style guarantee of immutability unnecessary. The primary exception is Http.Request, since that does need to store the data until the request is actually transmitted.
Though as far as Http.Request is concerned it might actually be possible or preferrable to make an entirely D-side version similar to how we handle Text.
I've also included a minor, but related change to the Joystick.Identification code; rather than allocating a new dstring every time an ID is requested, I'm caching them in an associative array by Vid and Pid, so that they only have to be fetched from C++ once.