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

[GL] Use StringView instead of std::string #149

Merged

Conversation

st0rmbtw
Copy link
Contributor

This PR is not much useful, just some code quality changes.

I'm not sure if I've done it right, but in theory it should reduce the amount of string copies.

Copy link
Owner

@LukasBanana LukasBanana left a comment

Choose a reason for hiding this comment

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

Looks like a neat improvement. Once StringLiteral was introduced in the public interface, all those std::string instantiations become merely transition variables that pass on their internal data but don't need to own the memory, so this change makes perfectly sense.

Please be so good and wrap all the nameLength into static_cast<std::size_t> to make the type conversion from GLint and GLsizei explicit. I was suprised the compilers don't warn, but I presume that's only happening on a higher warning level.

sources/Renderer/OpenGL/Shader/GLLegacyShader.cpp Outdated Show resolved Hide resolved
sources/Renderer/OpenGL/Shader/GLShaderProgram.cpp Outdated Show resolved Hide resolved
@LukasBanana LukasBanana self-assigned this Jan 19, 2025
@LukasBanana LukasBanana added the improvement General improvements and logical fixes. label Jan 19, 2025
@st0rmbtw st0rmbtw force-pushed the use_string_view_instead_of_std_string branch from 1307e2a to 8458ea3 Compare January 19, 2025 18:51
@st0rmbtw
Copy link
Contributor Author

I polluted the branch, so I created a new branch in my local fork, cherry-picked those two commits into that branch and forced pushed them into this remote branch.

Please, check if I've done it right.

@LukasBanana
Copy link
Owner

Looks good to me. I'll merge as soon as the unit tests succeed. GitHub has the Squash and merge operaton, so it shouldn't matter how many commits you pushed.

@LukasBanana LukasBanana merged commit d1b43a6 into LukasBanana:master Jan 19, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement General improvements and logical fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants