Utils: improve string methods #168
Conversation
WalkthroughThe changes update logging and string conversion utilities in the Utils namespace. Function signatures in both Utils.cpp and Utils.hpp have been modified to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Logger
participant Conversion
participant Mutex
Client->>Logger: Call Log(message)
Logger->>Mutex: Acquire lock with std::unique_lock
Logger->>Conversion: Convert message via NarrowToWideString
Conversion-->>Logger: Return wide string
Logger->>PlatformLogging: Forward converted message for logging
Mutex-->>Logger: Release lock
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
Utils.cpp (3)
4-10: Note on deprecated header usage.
The<codecvt>header is deprecated in C++17. Using it is acceptable for now but consider migrating to a more modern approach (e.g., Windows APIsMultiByteToWideChar/WideCharToMultiByteor other libraries) to ensure longer-term support.
24-28: Use safer formatting to avoid potential buffer overruns.
Althoughvsprintf_sis safer thanvsprintf, consider switching tovsnprintf_sor checking the return value to reduce any residual risk if the log message exceeds 2047 characters.- vsprintf_s(message.data(), message.size() - 1, fmt, list); + vsnprintf_s(message.data(), message.size(), _TRUNCATE, fmt, list);
74-78: Converting from wide to UTF-8 via<codecvt>is valid but deprecated.
Consider future-proofing by migrating off<codecvt>—especially since you’ve already added the suppression macro for deprecation warnings.Utils.hpp (1)
36-37: Centralizing encoding conversions.
IntroducingWideToNarrowStringandNarrowToWideStringclarifies the encoding workflow. Consider gradually phasing out<codecvt>if you need longer-term support.moonlight-xbox-dx.vcxproj (8)
152-153: Suppressing<codecvt>deprecation for ARM Debug.
This resolves compilation warnings but does not address the root cause of deprecation. Consider replacing<codecvt>in future.
167-168: Suppressing<codecvt>deprecation for ARM Release.
Same note as with Debug configuration: explore alternate conversions to remove the suppression macro.
182-183: Suppressing<codecvt>deprecation for ARM64 Debug.
Consistency maintained across all configurations. Replacing<codecvt>eventually will remove the need for repeated suppression macros.
197-198: Suppressing<codecvt>deprecation for ARM64 Release.
Same rationale applies: consider migrating to modern string conversion APIs.
212-213: Suppressing<codecvt>deprecation for Win32 Debug.
Keep caution: these macros might hide legitimate warnings if you switch to a non-deprecated method in the future.
227-228: Suppressing<codecvt>deprecation for Win32 Release.
Again, consistent approach, but a thorough replacement of<codecvt>remains advisable.
242-243: Suppressing<codecvt>deprecation for x64 Debug.
Your code remains portable for now, but note that<codecvt>might be removed in a future standard.
257-258: Suppressing<codecvt>deprecation for x64 Release.
Same recommendation applies: consider a more future-proof approach thanstd::wstring_convert.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Utils.cpp(2 hunks)Utils.hpp(1 hunks)moonlight-xbox-dx.vcxproj(8 hunks)
🔇 Additional comments (7)
Utils.cpp (5)
48-52: Overloading withconst char*is a good approach.
This implementation routesconst char*logs into the mainstd::string_viewoverload, which helps avoid code duplication.
58-64: Null handling is consistent and safe.
Returningnullptrappropriately mirrors a null input to avoid throwing an exception. This is a good defensive pattern in interop methods.
66-68: Efficient conversion fromstd::stringtoPlatform::String^.
The call toNarrowToWideStringis straightforward, ensuring minimal overhead for the conversion.
70-73: Direct conversion to UTF-8.
UsingWideToNarrowStringfor convertingPlatform::String^tostd::stringcentralizes logic and prevents duplicate code paths.
79-82: Symmetrical approach for string conversions.
UsingNarrowToWideStringin tandem withWideToNarrowStringis consistent and helps reduce confusion around Unicode conversions.Utils.hpp (2)
29-30: Good overload design forLog.
Having bothLog(const char* msg)andLog(const std::string_view& msg)maximizes convenience and consistency.
33-33: Changing toconst char*is a safer interface.
Preventing callers from modifying the buffer is beneficial for function clarity and correctness.
| void Log(const std::string_view& msg) { | ||
| try { | ||
| if (fmt == nullptr || fmt == NULL)return; | ||
| int len = strlen(fmt) + 1; | ||
| wchar_t* stringBuf = (wchar_t*)malloc(sizeof(wchar_t) * len); | ||
| if (stringBuf == NULL)return; | ||
| mbstowcs(stringBuf, fmt, len); | ||
| std::wstring string(stringBuf); | ||
| logMutex.lock(); | ||
| if (logLines.size() == LOG_LINES)logLines.erase(logLines.begin()); | ||
| logLines.push_back(string); | ||
| logMutex.unlock(); | ||
| OutputDebugStringA(fmt); | ||
| std::wstring string = NarrowToWideString(msg); | ||
| { | ||
| std::unique_lock<std::mutex> lk(logMutex); | ||
| if (logLines.size() == LOG_LINES) { | ||
| logLines.erase(logLines.begin()); | ||
| } | ||
| logLines.push_back(string); | ||
| } | ||
| OutputDebugString(string.c_str()); | ||
| } | ||
| catch (...){ | ||
| catch (...) { | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Avoid catching and ignoring all exceptions silently.
Swallowing exceptions completely may mask underlying issues. Log or otherwise handle exceptions within the catch block to ensure you can diagnose errors when they happen (e.g., logging error details).
try {
...
}
catch (...) {
- // currently doing nothing
+ // Possible improvement:
+ OutputDebugString(L"An exception occurred in Log()\n");
}Committable suggestion skipped: line range outside the PR's diff.
This is a first of multiple PRs with fixes and some changes. I am cherry-picking changes from my multiple-changes branch.
This PR:
Instead of fixing the current implementation, I am migrating to C++ RAII style. I find it is easier to reason about.
Additionally, I understand that codecvt got deprecated in C++17, and will be removed in C++... 26 I think. But its interface is so much easier to use than
MultiByteToWideCharandWideCharToMultiByte. I can modify the code to those Windows APIs if you prefer, though.I do not modify the other implementation files to properly use the new methods yet, but that PR will come if this is approved.
Summary by CodeRabbit
New Features
Refactor
Chores