RobLoach / raylib-cpp

C++ Object Oriented Wrapper for raylib
https://robloach.github.io/raylib-cpp/
zlib License
584 stars 77 forks source link

update const std::string& to const char* #292

Closed shawn-x3 closed 4 months ago

shawn-x3 commented 4 months ago

This library overuses std::string type, making a temporary copy every time you pass a const char into a const std::string& argument making unnecessary copies. It should support mainly const char, or support both with an intermediate struct.

kyomawolf commented 4 months ago

why would you be using char* anyway?

shawn-x3 commented 4 months ago

i would use const char mainly because there's no need of overusing the creation of std::string each time you call raylib::Texture2D texture("space.png") for example, it will copy your const char into a temporary std::string. Raylib sadly only accepts const char* and it would be really nice if raylib would accept std::string_view but raylib it's not a c++ library.

kyomawolf commented 4 months ago

I kind of see the point, but if I work with normal strings, this makes it weird and misses the point of having a c++ wrapper. I rather suggest doing it the proper c++17 way and using string_view instead

shawn-x3 commented 4 months ago

if you use string_view has no sense and it will cause bugs, you will need to pass .data() to raylib and there's no guarantee the string view is null terminated (e.g if you change its size, it will be ignored), the only solution is to combine const char and const std::string& with overload or using a custom stringptr struct which accepts const char or const std::string& or any other and gives their pointer

kyomawolf commented 4 months ago

That is a good point, then I would suggest this:


class s_string {
private:
const char* internal;
public:
s_string(const char *s) : internal(s) { }
s_string(std::string& s) : internal(s.c_str()) {}
}
shawn-x3 commented 4 months ago

yea, acceptable

class s_string {
    const char* m_ptr;
public:
     s_string(const char *s) : m_ptr(s) { }
     s_string(const std::string& s) : m_ptr(s.c_str()) {}
     operator const char*() { return m_ptr; }
};

We should ask @RobLoach

RobLoach commented 4 months ago

Making our own string wrapper class seems excessive when both string and string_view already exist?

Also note that raylib will eventually clear it's own internal char* string buffers itself, so we will need to be careful on what uses this.

shawn-x3 commented 4 months ago

@RobLoach you can't use string_view for raylib, because you can't specify to raylib the size of the actual string, because it's not even null terminated (you don't have .c_str() in string_view), and std::string would be a waste of performance copying const char* contents every time you need to call any function with const std::string&, that's why making our own string wrapper would be a good idea and would avoid any copy

RobLoach commented 4 months ago

Thanks for the clarification. After some thought on this, I think it may be better to overload the methods. Then if you pass in a const char*, it just calls DrawText directly without any new object at all or casting or anything.

void DrawText(const char* text, int posX = 0, int posY = 0, int fontSize = 10.0f) const {
    ::DrawText(text, posX, posY, fontSize, *this);
}

void DrawText(const std::string& text, int posX = 0, int posY = 0, int fontSize = 10.0f) const {
    ::DrawText(text.c_str(), posX, posY, fontSize, *this);
}
RobLoach commented 4 months ago

Added a few of the overloads, but let me know if there are any others that you think would benefit from it.