Bye bye std::string in the GL library #499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I have a GTX 1660 Ti, will that work? |
|
Yes, absolutely. I'll keep you in mind when I get back to this :) |
97 tasks
Open
mosra
force-pushed
the
gl-bye-bye-std-string
branch
19 times, most recently
from
c5fbb8a to
81d85ea
Compare
The previous callback pointer was needed just to disambiguate, so it can be a bool; the user pointer can be set from the caller already instead of being done in each variant again.
Those are reserved for temporary TODOs and debugging, not meant to stay there forever.
So far everything everywhere was just testing the success case, without checking that the message is properly retrieved and returned. Sigh.
The test added in previous commit passed on Mesa, but not on SwiftShader.
Most of these are just inputs, so a compatibility StringStl.h include will do, the only exception is the callback for which there needs to stay a deprecated overload (which is internally delegated from the StringView one). Also explicitly testing with non-null-terminated strings -- the APIs take an explicit size so it shouldn't be a problem, but it's always good to have this verified independently. Drivers are crap, you know. One consequence of no longer using an impossible-to-forward-declare std::string is that I had to deinline the DebugGroup constructor because it no longer worked with just a forward-declared StringView.
Do it the same as for the skinning stuff, format() and string multiplication. No need to get this crazy.
Same as in the previous commit, most cases are inputs so a StringStl.h compatibility include will do, the only breaking change is GL::Shader::sources() which now returns a StringIterable instead of a std::vector<std::string> (ew). Awesome about this whole thing is that The Shader API now allows creating a shader from sources coming either from string view literals or Utility::Resource completely without having to allocate any strings internally, because all those can be just non-owning references wrapped with String::nullTerminatedGlobalView(). The only parts which aren't references are the #line markers, but (especially on 64bit) those can easily fit into the 22-byte (or 10-byte on 32bit) SSO storage. Also, various Shader constructors and assignment operators had to be deinlined in order to avoid having to include the String header, which would be needed for Array destruction during a move. Co-authored-by: Hugo Amiard <hugo.amiard@wonderlandengine.com>
Given that I made a breaking change by returning Containers::String instead of a std::string, I can take it further and replace also std::pair with Containers::Pair -- it won't bring any more pain to the users, they have to change their code anyway. Co-authored-by: Hugo Amiard <hugo.amiard@wonderlandengine.com>
Apparently even `= {}` was broken for std::pair once, not to mention the
unnecessary extra overhead with this type not being trivially copyable.
Good riddance.
Hm, and here I used the "capability" of std::pair that allowed it to store references. I don't even want to know what all was involved to support that, Containers::Reference is much easier to reason about.
And this, this change allows the growable Array to use malloc() instead of new, and thus also realloc(), saving unnecessary reallocations if the memory can be grown in-place. All because Containers::Pair is trivially copyable while std::pair wasn't. There isn't any good reason to use the STL anymore.
Simple & easy, for a change.
One of the last remaining leftovers.
Or document why is it still needed.
From some, at least. The easings need to wait for a growable String.
Mainly important for Shader::addSource() to prevent it from creating a needless copy, but doesn't hurt to do the same also for uniformLocation(), bindAttributeLocation() etc. -- it'll avoid a runtime strlen() in that case at least.
Not built by default, but handy for profiling.
mosra
force-pushed
the
gl-bye-bye-std-string
branch
from
44b27f5 to
ac03515
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As usual, a Saturday afternoon task that turned out to cut much deeper than anticipated. Things to do:
label()/setLabel()away fromstd::string-- bc88442setLabel()/label()tests for all Query and Texture subclasses in order to verify these are correctly implemented and exported (since those are all deinlined in new files now)DebugOutput,DebugGroupaway fromstd::stringShaderandAbstractShaderProgramaway fromstd::string#linestatements added to shader sources needUtility::format()to return aString-- mosra/corrade@ea9f217Shader::addFile()needsUtility::Path::readString()to return aString-- mosra/corrade@a211a75Globalflag fromString::nullTerminatedGlobalView()-- mosra/corrade@f65c652variousnot anymore, 65a935c<string>includes may need to be added to various placesCORRADE_SKIP()to not need them, because that's the major reason for these -- mosra/corrade@8d077ccstd::pairfromAbstractShaderProgram::validate()as it's a breaking change already anywayShader::compile()to use an ArrayTuple for the output instead of two allocationsUtility::Resourcestarts supporting null-terminated views -- mosra/corrade@9ef4b4aRelated to: #293