-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Conversation
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.
Minor issues with code clarity, otherwise fine.
- It seems like many of the
const std::string&
can be replaced withstd::string_view
with no impact on code clarity, but a positive impact on performance. auto
is fine if used with specifies likeconst
,&
and*
for code clarity. Always provide the full intent behind the code to both the reader and the compiler.- Formatting of multiple files is incorrect - ensure you have the latest LLVM/Clang version installed and have run it.
- The commits are formatted incorrectly: https://github.com/Xaymar/obs-StreamFX/blob/master/CONTRIBUTING.md#commits
FWIW, the use of |
@Xaymar,
@Xaymar, 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 |
In the example you provided you can either opt for 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 |
@Xaymar, should |
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:
|
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.
Remove the translation files, fix the commit titles and descriptions and this is good.
Will test this on all platforms this weekend. If it fails, I'll reject the PR, if it works, I'll merge. |
@Xaymar, hi |
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. |
- Use auto in places where code clarity is improved or identical. - Replace trivial constructors and destructors with default. - Use true random for random generation. - Use std::string_view where it is valid to do so. - Apply const where it is valid to do so. - Use references where it is valid to do so. - Manually optimize memory usage with std::move and std::copy. - Opt for memory efficient containers where the size is known ahead of time. Signed-off-by: lainon <[email protected]>
LGTM |
The use of const references breaks '#include' for relative paths.
The use of const references breaks '#include' for relative paths.
The use of const references breaks '#include' for relative paths.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
It is not valid to pass std::string_view to snprintf's %s.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
It is not valid to pass std::string_view to snprintf's %s.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
The use of const references breaks '#include' for relative paths.
It is not valid to pass std::string_view to snprintf's %s.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
The use of const references breaks '#include' for relative paths.
It is not valid to pass std::string_view to snprintf's %s.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
The use of const references breaks '#include' for relative paths.
It is not valid to pass std::string_view to snprintf's %s.
The compiler will choose the optimal way automatically, and forcing std::move here actually results in two objects existing side by side, before being "moved" into one.
Explain the Pull Request
Minor optimizations and code simplification. Bringing to a modern C++ standard.
Related Issues
None
Checklist