Fix: Fix leak #89
Fix: Fix leak #89
Conversation
WalkthroughThe changes in the OpenDDLParser.cpp file focus on enhancing error logging functionality by modifying the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (3)
code/OpenDDLParser.cpp (3)
77-84: Consider early return and string view optimization.While the change to std::string is safer, there are a few potential improvements:
- Add early return if callback is null to avoid unnecessary string operations
- Consider using std::string_view (C++17) for better performance
- The magic number 50 should be a named constant
+static const size_t MAX_ERROR_PREVIEW_LENGTH = 50; + static void logInvalidTokenError(const std::string &in, const std::string &exp, OpenDDLParser::logCallback callback) { + if (!callback) { + return; + } - if (callback) {\ - std::string part(in.substr(0, 50)); - std::stringstream stream; - stream << "Invalid token \"" << in << "\" " - << "(expected \"" << exp << "\") " - << "in: \"" << part << "\""; - callback(ddl_error_msg, stream.str()); - } + std::string part(in.substr(0, MAX_ERROR_PREVIEW_LENGTH)); + std::stringstream stream; + stream << "Invalid token \"" << in << "\" " + << "(expected \"" << exp << "\") " + << "in: \"" << part << "\""; + callback(ddl_error_msg, stream.str()); }
308-308: Consider optimizing string construction in error logging calls.The current implementation creates temporary strings for each error log. Consider using string_view or passing the length explicitly to avoid unnecessary memory allocations.
- logInvalidTokenError(std::string(in, end), Grammar::ClosePropertyToken, m_logCallback); + logInvalidTokenError({in, static_cast<size_t>(end - in)}, Grammar::ClosePropertyToken, m_logCallback); - logInvalidTokenError(std::string(in, end), std::string(Grammar::OpenBracketToken), m_logCallback); + logInvalidTokenError({in, static_cast<size_t>(end - in)}, Grammar::OpenBracketToken, m_logCallback); - logInvalidTokenError(std::string(in, end), std::string(Grammar::CloseBracketToken), m_logCallback); + logInvalidTokenError({in, static_cast<size_t>(end - in)}, Grammar::CloseBracketToken, m_logCallback);Also applies to: 357-357, 428-428
Line range hint
1-1000: Consider broader memory safety improvements.While the changes improve error logging safety, there are several areas where memory safety could be further enhanced:
- Consider using smart pointers instead of raw pointers for memory management
- Add bounds checking for pointer arithmetic operations
- Consider using string_view for string operations that don't need ownership
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/OpenDDLParser.cpp(5 hunks)
🔇 Additional comments (1)
code/OpenDDLParser.cpp (1)
33-35: LGTM! Good practice for header guard implementation.
The addition of the header guard for WIN32_LEAN_AND_MEAN prevents potential redefinition warnings while maintaining the intended functionality.
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Summary by CodeRabbit