-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
PathUtil std::string support #7246
base: master
Are you sure you want to change the base?
Conversation
- Add std::string/std::string_view support - Add "Internal" Base location for referring to internal DSO locations
I have doubts about Windows compatibility and the use of |
@sakertooth Windows shouldn't be an issue. When I convert from |
The issue with narrow strings on Windows only occurs when calling Windows API functions. As long as the strings are converted to wide strings before they're passed to the OS, there's no issue with using UTF-8 internally. |
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.
Adds UTF8-encoded std::string/std::string_view support to
PathUtil
I would prefer we prefix the UTF-8 based functions with ut8
so its clear that the result from these functions are a std::string
/std::string_view
storing a UTF-8 string. That being said, stuff like basePrefixQString
can just be left as basePrefix
, while your new function would then be named utf8BasePrefix
(If we had C++20 support, we could return a std::u8string
for even more clarity).
It also might be easier to wrap around the QString
based functions rather than re-implementing them to use UTF-8 and going the other way around (Naturally, I thought you were implementing your new functions based on the QString
versions. It didn't click at first, and so I thought your new functions were just duplicating the QString
ones).
Adds "Internal" Base location for referring to internal DSO locations (needed for CLAP presets stored inside CLAP plugins)
Also, could you elaborate what you mean by DSO locations? The abbreviation is not as clear to me (dynamic shared objects?).
I would argue the other way - the aim is eventually to remove Qt from the core, so the default choice should be to use UTF-8-encoded |
Then we should remove the I simply don't see the point keeping the |
- Remove PathUtil::basePrefixQString() - Clarify the usage of UTF-8
@sakertooth I just removed
Yes, dynamic shared objects such as .dll and .so. CLAP presets can use a pseudo path which is used to identify a preset stored within the CLAP plugin DSO itself rather than on the file system, and by giving those paths |
Potentially out of scope: I assume the |
That's better, but I'm still confused as to why we need to keep the other Edit: Unless you want callers to explicitly specify which one to use with |
@sakertooth I'll see about removing some of the other |
I'm moving some of the additions from #7199 into this separate PR to make reviewing the CLAP PR easier.
PathUtil