GL: Cleanup std::string usage in Shader/AbstractShaderProgram

[hugoam]

I stumbled on this remaining usage of std::string, std::vector and std::pair so I'm giving a shot at migrating it.

Not sure exactly yet what's the backwards compatibility strategy to employ here, so I'm kind of opening this PR to figure it out, hope it's not too much of a bother!

[mosra]

Umm, I have a WIP branch with very similar changes already (#499), just haven't gotten around to finish it because so many things to do.

Which means, my review of this PR would be basically making it look exactly the same as my local WIP changes, so ... :sweat_smile: ... to save us both time, can you just wait until I finish #499?

EDIT: Just the backwards compatibility considerations re std::pair in particular are a big task on its own, and I don't even know how to handle that yet. Probably will do that only once all std::string backwards compat is finished everywhere, to avoid having too many breaking changes at once.

[mosra]

This is now integrated in db2cb492797ac3379b843f3c31ff3fbe8114832f and d0e18e1f5c50c1043aec596ab5084853228db938 as part of #499. It ended up being significantly different from your changes (especially the copy-avoiding internals in GL::Shader), but since you spent the time on this anyway, I added you as a co-author at least :)

[hugoam]

Amazing that this got merged, and that you added me as co-author, hehe, I didn't expect that much ! 😄

[mosra]

Yeah sorry the cleanup took so long, on the other hand it relies heavily on quite recent features such as the StringIterable or the Global flag getting preserved in a String, which just weren't there back in March 2021 when I started #499 :)

[hugoam]

I also just realised the Utility::String namespace still contains some useful functions which are std::string based like replaceAll(). Are there some alternatives to those already elsewhere, or is the intention to port those to String one day?

[mosra]

The intention is to port those to String eventually, yes. I didn't do that yet because some of these need growable support in String first -- i.e., replaceAll() could be done in two passes where I first find all occurences, calculate the output size, and then fill it in a second pass, but it would be much slower than just finding & populating a growable output in one go. (Though it could internally abuse a growable Array for now and then the memory be transferred to a String, maybe I'll do just that.) What was easy to port is ported already, either as a new overload in Utility::String or directly on the Containers::String[View] class (such as the trimming, splitting and joinint APIs).

I'm painfully aware of all remaining yet-unported locations -- there's still PluginMetadata, Arguments the whole Configuration class and (ugh) the Text library, but for all of those I still need to figure out some design questions first, neither of them is a trivial port. Next I hope I'll tackle the Arguments, and Configuration after that.

[hugoam]

I see. Another one I just noticed: the setFileCallback callbacks in AbstractImporter, AbstractFont, etc. I'm wondering, isn't it theoretically easy to fix those in a backwards compatible way by just replacing with StringView and including StringStl.h in those headers when building with DEPRECATED=1? I was assuming the implicit conversions will work, probably even for callbacks? One problem I see is probably plugins implementers but maybe some breakage could be accepted there? If you think that's indeed as simple one that I could tackle as opposed to the ones you cited, maybe I can help with that

[mosra]

Those can't be changed in a backwards-compatible way, as it'd be a different function signature. But I solved the exact same problem for GL::DebugOutput::setOutputCallback() already in d89b882ccf4d4af6c4a1f9e9c7ef63ddffaee85b (keeping the old signature but marking it deprecated, and delegating to it from the new one) so this would be similar, nothing complicated.

The main reason I didn't change those yet was that the callbacks need an upgrade anyway, to support what AbstractImporter::openMemory() allows, i.e. being able to tell the importer that the file contents are guaranteed to stay in scope (so the importer doesn't need to make copy), or, conversely, that the importer should make a copy of them, or even some way to transfer the data ownership to the importer. And similarly in the other direction, the importer asking for something like "give me the contents so I can directly reference them without making a copy if possible, and tell me if you weren't able". Basically to support something like what's outlined in #240, but in a generic way. Currently it's possible only for the top-level file via openMemory(), but not for subsequent files, and that's limiting.

Yeah so I didn't change those yet because when I would update them to implement the above, it would be a second breakage for the users, forcing them to adapt all their callback function signatures yet again. Better to just break once than twice, in my opinion.

Sorry that there isn't any low-hanging fruit left :sweat_smile: