obs-StreamFX
obs-StreamFX copied to clipboard
Xaymar
Working on source code [Refactor]
Explain the Pull Request
Minor optimizations and code simplification. Bringing to a modern C++ standard.
Related Issues
None
Checklist
- [ ] I will become the maintainer for this part of code.
- [ ] I have tested this code on all supported Platforms.
- [X] I have tested this code on Windows.
FWIW, the use of auto is fine, but the use of explicit types should be preferred for code clarity. As it is, this PR sacrifices code clarity for slightly more compiler flexibility.
@Xaymar,
OK, I'd rather replace const std::string& with std::string_view. It will be better for updating the code to C++17
I will correct the formatting of the code, today I will correct the code according to your requirements with your authorship
FWIW, the use of
autois fine, but the use of explicit types should be preferred for code clarity. As it is, this PR sacrifices code clarity for slightly more compiler flexibility.
@Xaymar,
If the function returns const char*, applying the type to the result variable can have a positive effect on optimization, the compiler itself will determine that it is better to use C-string, std::string or std::string_view, but provided that no one explicitly overrides to another type. const auto or auto in modern C++ code is already the norm for iterators, lambdas, range-based for loop, it's worth getting used to.
Example:
how readable is the code?
before:
std::map<int, std::string> myMap;
myMap.emplace(0, "Ergo");
myMap.emplace(1, "Proxy");
for (std::map<int, string>::iterator it = myMap.begin(); it != myMap.end(); ++it){
std::cout << it->first<< " has value " << it->second << std::endl;
}
after:
std::map<int, std::string> myMap;
myMap.emplace(0, "Ergo");
myMap.emplace(1, "Proxy");
for (const auto& [key, value]: myMap) {
std::cout << key << " has value " << value << std::endl;
}
In terms of readability in loops, variables were assigned as key and value rather than it->first and it->second.
Writing smaller code allows you to achieve a similar algorithm without iterators.
how readable is the code?
In the example you provided you can either opt for decltype(...)::iterator or const auto&, but not auto. This is because auto by itself drastically reduces code clarity since it does not convey any information other than "Try guessing what this will be!". However this example doesn't even remotely compare to the errors made in this Pull Request.
We are dealing with a third party API here, which if changed could drastically change how the code behaves. While we can somewhat guarantee that it won't happen, we still want the compiler to pick it up at the point where we use the API - and not any later. This also helps with code clarity as a reader would not have to open the API documentation to figure out the return type.
Code Clarity is always about explicitly stating intent. When the options are unclear intent and human-decoding taking longer, or clear intent and human-decoding is instant, the latter option is always preferred if not even required. This project isn't the only project following this rule - many other massive open source projects follow the same pattern.
Forgot to mention the other problem of const char* possibly being a nullptr, thus invoking undefined behavior in many compilers and libraries that are still being supported. Therefore the use of auto is even more dangerous when the compiler starts assuming that functions which return const char* will never return nullptr, and replaces it with std::string_view instead.
should const char* be changed to std::string_view?
Only if we control the entire initialization part of std::string_view. It should not be changed to std::string_view if it faces an external API which may return nullptr (not supported by the C++17 standard, or all C++17 compatible libraries). So:
std::string_view potential_nullptr = obs_data_get_string(data, "text")); // No, not supported by the standard without extensions.
const char* potential_nullptr = obs_data_get_string(data, "text")); // Yes.
std::string_view technique = "Draw"; // Also yes, no external API dependency.
Will test this on all platforms this weekend. If it fails, I'll reject the PR, if it works, I'll merge.
@Xaymar, hi I've been busy for two weeks, I can fix remaining changes if necessary
There's only a rebase left, as well as verifying that the & doesn't cause memory corruptions.
As I'm preparing to build for Qt6+OBS Studio 28.0, testing for this is a bit delayed still. I handled the rebasing and commit style though, so that part is done.