Add argument parser #1
Conversation
WalkthroughAdds ArgumentParser and Tokenizer core utilities and registers them in the runtime build; introduces a StringArray alias; small app shutdown/logging adjustments; adds RenderPipelineState type; updates cppcore submodule pointer; removes a swapchain creation call from Vulkan init. Changes
Sequence Diagram(s)(No sequence diagram generated — changes do not introduce a new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/renderer/RHIVulkan.cpp (1)
334-369: Logical device queue creation uses only one queue create infoThe switch to
cppcore::TArrayis fine, but theVkDeviceCreateInfowiring is now inconsistent:
- You build
queueCreateInfosfor all unique queue families.- Then you set
createInfo.queueCreateInfoCountfromqueueCreateInfos.size()andcreateInfo.pQueueCreateInfos = queueCreateInfos.data();- Finally,
createInfo.queueCreateInfoCountis overwritten to1, so only the first queue create info is actually used, even if there are two unique families (graphics + present).- The separate
queueCreateInfo/queuePrioritysblock is now redundant and confusing.This can result in missing queues on devices where graphics and present families differ.
A minimal fix is to drop the redundant local
queueCreateInfoblock and consistently usequeueCreateInfos:- VkDeviceQueueCreateInfo queueCreateInfo{}; - queueCreateInfo.sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO; - queueCreateInfo.queueFamilyIndex = queueFamilyIndices.graphicsFamily.value(); - queueCreateInfo.queueCount = 1; - - // priorities - float queuePrioritys[2] = { 1.f, 1.f }; - - queueCreateInfo.pNext = nullptr; - queueCreateInfo.pQueuePriorities = &queuePrioritys[0]; - VkPhysicalDeviceFeatures deviceFeatures{}; VkDeviceCreateInfo createInfo{}; createInfo.sType = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO; - - createInfo.pNext = nullptr; - createInfo.pQueueCreateInfos = &queueCreateInfo; - - createInfo.queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()); - createInfo.pQueueCreateInfos = queueCreateInfos.data(); - - createInfo.queueCreateInfoCount = 1; + + createInfo.pNext = nullptr; + createInfo.queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()); + createInfo.pQueueCreateInfos = queueCreateInfos.data();This makes the advertised queues match the
uniqueQueueFamiliesyou collected.
🧹 Nitpick comments (3)
src/runtime/application/app.cpp (1)
124-135: Early return inApp::shutdowncan skip parts of shutdownThe new guard
if (mSdlWindow == nullptr) { logMessage(LogType::Error, "Invalid application state, cannot shutdown."); return; }avoids destroying a null window, but it also prevents the rest of the shutdown path (e.g.,
SDL_Quit()and any future teardown you add here) from running ifmSdlWindowis unexpectedly null.If
App::shutdownis intended to be robust/idempotent, consider:
- Logging the error but still calling
SDL_Quit()(and any RHI teardown) when appropriate, or- Narrowing the guard to just skip
SDL_DestroyWindowwhile allowing the rest of shutdown to proceed.src/runtime/core/argumentparser.cpp (2)
23-33: Include standard headers for C library functions used in this TUThis file uses several C library functions:
::strlen(constructor)memset,sprintf(showHelp)atoi(parseArgParameter)but only includes project headers. Relying on transitive includes from other headers makes the translation unit fragile.
Make the file self-contained by adding the relevant standard headers:
#include "core/argumentparser.h" #include "core/tokenizer.h" + +#include <cstring> // strlen, memset +#include <cstdio> // sprintf +#include <cstdlib> // atoi
44-71: Clean up redundant pre-tokenization size check inparseExpectedArgsIn
parseExpectedArgsyou have:StringArray extractedArgs, extractedDescs; if (extractedDescs.size() != extractedArgs.size()) { error += "Missing description for arguments"; return false; } Tokenizer::tokenize(userDefinedArgs, extractedArgs, ":"); Tokenizer::tokenize(desc, extractedDescs, ":"); uint32_t numParam(0); if (extractedArgs.size() != extractedDescs.size()) { error += "Number of arguments and descriptions do not match"; return false; }The first size comparison is always false (both arrays are just constructed and empty), so that branch is dead code. The real validation happens after tokenization.
You can simplify this helper by dropping the unused pre-check:
- StringArray extractedArgs, extractedDescs; - if (extractedDescs.size() != extractedArgs.size()) { - error += "Missing description for arguments"; - return false; - } - - Tokenizer::tokenize(userDefinedArgs, extractedArgs, ":"); - Tokenizer::tokenize(desc, extractedDescs, ":"); + StringArray extractedArgs, extractedDescs; + Tokenizer::tokenize(userDefinedArgs, extractedArgs, ":"); + Tokenizer::tokenize(desc, extractedDescs, ":");This keeps behaviour unchanged while making the code clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/runtime/CMakeLists.txt(1 hunks)src/runtime/application/app.cpp(2 hunks)src/runtime/core/argumentparser.cpp(1 hunks)src/runtime/core/argumentparser.h(1 hunks)src/runtime/core/filearchive.h(0 hunks)src/runtime/core/segfault.h(2 hunks)src/runtime/core/tokenizer.cpp(1 hunks)src/runtime/core/tokenizer.h(1 hunks)src/runtime/renderer/RHIVulkan.cpp(2 hunks)src/runtime/renderer/rendercore.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/runtime/core/filearchive.h
🧰 Additional context used
🧬 Code graph analysis (5)
src/runtime/application/app.cpp (1)
src/runtime/core/segfault.h (1)
logMessage(60-75)
src/runtime/core/tokenizer.h (1)
src/runtime/core/tokenizer.cpp (14)
getNumOfToken(9-11)getNumOfToken(9-9)getTokenAt(13-19)getTokenAt(13-13)isEmpty(21-23)isEmpty(21-21)clear(25-27)clear(25-25)tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)isLineBreak(62-68)isLineBreak(62-62)
src/runtime/core/segfault.h (1)
src/runtime/core/argumentparser.h (2)
string(78-158)string(160-170)
src/runtime/core/argumentparser.cpp (2)
src/runtime/core/tokenizer.cpp (6)
tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)clear(25-27)clear(25-25)src/runtime/core/argumentparser.h (2)
string(78-158)string(160-170)
src/runtime/core/argumentparser.h (1)
src/runtime/core/argumentparser.cpp (28)
ArgumentParser(73-128)Argument(34-37)Argument(39-42)showHelp(130-157)showHelp(130-130)getNext(159-170)getNext(159-159)clear(172-178)clear(172-172)isSupported(180-190)isSupported(180-180)getNumValues(192-206)getNumValues(192-192)argument(86-86)getArgument(208-221)getArgument(208-208)hasArgument(223-232)hasArgument(223-223)hasValidArgs(234-236)hasValidArgs(234-234)parseArgParameter(238-271)parseArgParameter(238-238)getBlankArgument(273-294)getBlankArgument(273-273)validateArguments(296-319)validateArguments(296-296)setInvalid(321-323)setInvalid(321-321)
🪛 Clang (14.0.6)
src/runtime/core/tokenizer.h
[error] 3-3: 'core/segfault.h' file not found
(clang-diagnostic-error)
src/runtime/core/tokenizer.cpp
[error] 1-1: 'core/tokenizer.h' file not found
(clang-diagnostic-error)
src/runtime/core/argumentparser.cpp
[error] 23-23: 'core/argumentparser.h' file not found
(clang-diagnostic-error)
src/runtime/core/argumentparser.h
[error] 25-25: 'core/segfault.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (3)
src/runtime/renderer/rendercore.h (1)
24-29: EmptyRenderPipelineStateplaceholder is fineThe empty struct is a reasonable hook for future pipeline configuration attached to
DrawCommand. No functional issues as-is.src/runtime/core/segfault.h (1)
5-6:StringArrayalias and TArray include look appropriatePulling in
cppcore::TArrayhere and exposingusing StringArray = cppcore::TArray<std::string>;insegfault::corematches howArgumentParserandTokenizeruse string arrays and keeps the container choice centralized.Also applies to: 77-78
src/runtime/CMakeLists.txt (1)
1-11: Core sources correctly wired intosegfault_runtimeIncluding
argumentparserandtokenizer(both.hand.cpp) insegfault_core_srccleanly integrates the new core utilities into the runtime library.
| /// @brief Returns next argument and its content. | ||
| /// @param arg [in] Next argument. | ||
| /// @param value [in] Content of argument. | ||
| /// @return true, if another arguments are supported. | ||
| bool getNext(std::string &arg, std::string &value ); | ||
|
|
There was a problem hiding this comment.
Make getNext iterate over detected arguments, not all supported ones
According to the header, getNext should iterate over the arguments actually detected on the command line. In argumentparser.cpp (lines 159‑169), the current implementation is:
bool ArgumentParser::getNext(std::string &arg, std::string &value) {
if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() ||
mCurrentIndex >= mSupportedArguments.size()) {
return false;
}
arg = mSupportedArguments[mCurrentIndex].mArgument;
value = mStoredArguments[mCurrentIndex];
++mCurrentIndex;
return true;
}Issues:
- Iteration limit is
mSupportedArguments.size(), but values are stored only for detected arguments, so indexes can go pastmStoredArguments.size(). argcomes frommSupportedArguments, whilevaluecomes frommStoredArguments; if some supported options were not passed, they will be paired with unrelated values.
Use the detected list as the iteration basis:
-bool ArgumentParser::getNext(std::string &arg, std::string &value) {
- if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() ||
- mCurrentIndex >= mSupportedArguments.size()) {
+bool ArgumentParser::getNext(std::string &arg, std::string &value) {
+ if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() ||
+ mCurrentIndex >= mDetectedArgs.size()) {
return false;
}
- arg = mSupportedArguments[mCurrentIndex].mArgument;
+ arg = mDetectedArgs[mCurrentIndex];
value = mStoredArguments[mCurrentIndex];
++mCurrentIndex;
return true;
}This keeps arg and value aligned and prevents out-of-bounds access on mStoredArguments.
🤖 Prompt for AI Agents
In src/runtime/core/argumentparser.h around lines 88–93: getNext currently
iterates over mSupportedArguments and pairs entries from mSupportedArguments
with values from mStoredArguments, which can cause out-of-bounds access and
misaligned pairs; change getNext to iterate over the detected/stored arguments
list instead (use mStoredArguments.size() as the loop bound) and return both arg
and value from the same detected-entry container (i.e., fetch the argument name
from the detected-arguments list that corresponds to mStoredArguments at the
same index, or store arg/value pairs together), and keep appropriate
empty/bounds checks and increment mCurrentIndex accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/core/argumentparser.h (1)
88-93: MakegetNextiterate over detected arguments to avoid misalignment / OOBThe header documents
getNextas iterating over “next argument and its content”, but the cpp currently does:bool ArgumentParser::getNext(std::string &arg, std::string &value) { if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || mCurrentIndex >= mSupportedArguments.size()) { return false; } arg = mSupportedArguments[mCurrentIndex].mArgument; value = mStoredArguments[mCurrentIndex]; ++mCurrentIndex; return true; }This iterates up to
mSupportedArguments.size()while values come frommStoredArguments(which only has detected args). If some supported options were not passed,mCurrentIndexcan exceedmStoredArguments.size()(OOB) and/or pair the wrongargwith avalue.Suggested fix (in
argumentparser.cpp), using the detected list consistently:-bool ArgumentParser::getNext(std::string &arg, std::string &value) { - if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || - mCurrentIndex >= mSupportedArguments.size()) { +bool ArgumentParser::getNext(std::string &arg, std::string &value) { + if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() || + mCurrentIndex >= mDetectedArgs.size() || + mCurrentIndex >= mStoredArguments.size()) { return false; } - arg = mSupportedArguments[mCurrentIndex].mArgument; + arg = mDetectedArgs[mCurrentIndex]; value = mStoredArguments[mCurrentIndex]; ++mCurrentIndex; return true; }Minor: the Doxygen on Line 89/90 tags
argandvalueas[in], but they are really output parameters; switching to[out](or[in,out]if you ever use them that way) would better reflect the API.
🧹 Nitpick comments (2)
src/runtime/core/argumentparser.h (2)
124-128: AlignparseArgParameterparameter naming with the Doxygen commentThe declaration uses
uint32_t &numParabut the Doxygen comment refers tonumParam, which is likely to trigger Doxygen warnings and makes the docs slightly misleading.You can either rename the parameter or fix the comment. Renaming for consistency is probably clearer:
- static bool parseArgParameter( const std::string &arg, uint32_t &numPara ); + static bool parseArgParameter( const std::string &arg, uint32_t &numParam );Don’t forget to update the definition in
argumentparser.cppaccordingly.
156-158: Considerstd::string_viewforaddErrorparameter (perf / flexibility)Sonar suggests replacing
const std::string &withstd::string_viewhere. Since you only append and don’t store the reference,string_viewwould avoid temporary allocations for string literals and allow more flexible callers.Within this header:
-inline void ArgumentParser::addError(const std::string &error) { +inline void ArgumentParser::addError(std::string_view error) { mLastError += error; }And add the appropriate include near the top of the file:
#include <string> #include <string_view>(Assumes the project is built with at least C++17; otherwise, keeping
const std::string &is fine.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/runtime/core/argumentparser.h(1 hunks)src/runtime/core/segfault.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/core/segfault.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/core/argumentparser.h (1)
src/runtime/core/argumentparser.cpp (28)
ArgumentParser(73-128)Argument(34-37)Argument(39-42)showHelp(130-157)showHelp(130-130)getNext(159-170)getNext(159-159)clear(172-178)clear(172-172)isSupported(180-190)isSupported(180-180)getNumValues(192-206)getNumValues(192-192)argument(86-86)getArgument(208-221)getArgument(208-208)hasArgument(223-232)hasArgument(223-223)hasValidArgs(234-236)hasValidArgs(234-234)parseArgParameter(238-271)parseArgParameter(238-238)getBlankArgument(273-294)getBlankArgument(273-273)validateArguments(296-319)validateArguments(296-296)setInvalid(321-323)setInvalid(321-321)
🪛 Clang (14.0.6)
src/runtime/core/argumentparser.h
[error] 25-25: 'core/segfault.h' file not found
(clang-diagnostic-error)
🪛 GitHub Check: SonarCloud Code Analysis
src/runtime/core/argumentparser.h
[warning] 156-156: Replace this const reference to "std::string" by a "std::string_view".
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (ubuntu-latest, Release, clang)
- GitHub Check: build (ubuntu-latest, Release, gcc)
🔇 Additional comments (1)
src/runtime/core/argumentparser.h (1)
25-25: Verify include path forsegfault.h("core/segfault.h"vs"segfault.h")Clang in the static analysis report can’t find
"core/segfault.h". Please double‑check that your include paths make this header visible from all translation units, or switch to including it relative to this header’s directory if the file really issrc/runtime/core/segfault.h.If it’s in the same directory as this header, one option would be:
-#include "core/segfault.h" +#include "segfault.h"
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/runtime/core/tokenizer.cpp (1)
13-19: Inverted bounds check makes this function always return EmptyToken.The condition
m_Token.size() >= indexis incorrect. For any valid index (e.g., index=0 when size=5), this evaluates to true (5 >= 0), causing the function to always return EmptyToken instead of the actual token.Apply this diff to fix the bounds check:
const std::string &Tokenizer::getTokenAt( uint32_t index ) const { - if ( m_Token.isEmpty() || m_Token.size() >= index ) { + if ( m_Token.isEmpty() || index >= m_Token.size() ) { return EmptyToken; } return m_Token[ index ]; }src/runtime/core/argumentparser.cpp (2)
69-78: Guard value-checking loop against reading pastargc.The loop iterates from
mCurrentIndex + 1tomCurrentIndex + numValues + 1without verifying that these indices are within bounds. IfnumValuesis large or there are insufficient arguments remaining,ppArgv[valIdx]will access memory beyondargc, causing undefined behavior.Apply this diff to add bounds checking:
if (const uint32_t numValues = getNumValues(argument)) { + // Ensure sufficient arguments remain + if (mCurrentIndex + numValues >= static_cast<uint32_t>(argc)) { + setInvalid(); + } else { // Check the number of expected values for being a new option - for (uint32_t valIdx = mCurrentIndex + 1; valIdx < mCurrentIndex + numValues + 1; valIdx++) { + for (uint32_t valIdx = mCurrentIndex + 1; + valIdx <= mCurrentIndex + numValues; + ++valIdx) { std::string tmpVal(ppArgv[valIdx]); std::string::size_type pos1 = tmpVal.find(option); if (pos1 != std::string::npos) { setInvalid(); break; } - } + } + } }
137-148: Iterate over detected arguments, not supported arguments.
getNextshould iterate over the arguments actually detected on the command line. Currently it usesmSupportedArgumentsas the iteration basis (line 139, 143) and pairs entries frommSupportedArgumentswith values frommStoredArguments. If some supported options were not passed, this causes:
- Out-of-bounds access when
mCurrentIndexexceedsmStoredArguments.size()- Misaligned argument-value pairs
Apply this diff to use detected arguments:
bool ArgumentParser::getNext(std::string &arg, std::string &value) { - if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || - mCurrentIndex >= mSupportedArguments.size()) { + if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() || + mCurrentIndex >= mDetectedArgs.size()) { return false; } - arg = mSupportedArguments[mCurrentIndex].mArgument; + arg = mDetectedArgs[mCurrentIndex]; value = mStoredArguments[mCurrentIndex]; ++mCurrentIndex; return true; }
🧹 Nitpick comments (3)
src/runtime/core/tokenizer.cpp (1)
62-68: Verify that Unix-only line break detection is sufficient.The function only recognizes
\nas a line break, ignoring\r(old Mac) and\r\n(Windows). Ensure this behavior aligns with your requirements.If cross-platform line break support is needed, apply this diff:
bool Tokenizer::isLineBreak(char c) { - if ( c == '\n' ) { - return true; - } - - return false; + return (c == '\n' || c == '\r'); }src/runtime/core/argumentparser.cpp (2)
126-131: Consider usingstd::to_stringinstead ofsprintf.The fixed-size buffer and
sprintfare C-style. For simple integer formatting,std::to_stringis cleaner and safer.Apply this diff:
if (numargs > 0) { helpMsg += "\n\tNumber of arguments: "; - char buffer[512]; - memset(buffer, '\0', 512); - sprintf(buffer, "%d\n", numargs); - helpMsg += std::string(buffer); + helpMsg += std::to_string(numargs); + helpMsg += "\n"; }
251-272: Simplify using the position already found.The code at line 260 finds the position of
tokenArgInbut doesn't use it. Instead, lines 266-269 loop again to find the same position. While safe (since existence is verified), this is redundant.Apply this diff:
std::string ArgumentParser::getBlankArgument(const std::string &argument) { - std::string blankArg(""); - // Empty, return it if (argument.empty()) { - return blankArg; + return ""; } // Check if any token for parameter definition is there - if (std::string::size_type pos = argument.find(tokenArgIn); std::string::npos == pos) { + const std::string::size_type pos = argument.find(tokenArgIn); + if (std::string::npos == pos) { return argument; } - // Clean from parameter-specific token - uint32_t i(0); - while (argument[i] != tokenArgIn) { - blankArg += argument[i]; - ++i; - } - - return blankArg; + // Return substring up to the token + return argument.substr(0, pos); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/runtime/core/argumentparser.cpp(1 hunks)src/runtime/core/argumentparser.h(1 hunks)src/runtime/core/filearchive.h(1 hunks)src/runtime/core/genericfilemanager.h(1 hunks)src/runtime/core/ifilemanager.h(1 hunks)src/runtime/core/tokenizer.cpp(1 hunks)src/runtime/core/tokenizer.h(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/runtime/core/genericfilemanager.h
- src/runtime/core/ifilemanager.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/core/filearchive.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/runtime/core/tokenizer.h (1)
src/runtime/core/tokenizer.cpp (14)
getNumOfToken(9-11)getNumOfToken(9-9)getTokenAt(13-19)getTokenAt(13-13)isEmpty(21-23)isEmpty(21-21)clear(25-27)clear(25-25)tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)isLineBreak(62-68)isLineBreak(62-62)
src/runtime/core/argumentparser.cpp (2)
src/runtime/core/tokenizer.cpp (6)
tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)clear(25-27)clear(25-25)src/runtime/core/argumentparser.h (2)
string(56-136)string(138-148)
🪛 Clang (14.0.6)
src/runtime/core/tokenizer.h
[error] 3-3: 'core/segfault.h' file not found
(clang-diagnostic-error)
src/runtime/core/argumentparser.h
[error] 3-3: 'core/segfault.h' file not found
(clang-diagnostic-error)
src/runtime/core/argumentparser.cpp
[error] 1-1: 'core/argumentparser.h' file not found
(clang-diagnostic-error)
src/runtime/core/tokenizer.cpp
[error] 1-1: 'core/tokenizer.h' file not found
(clang-diagnostic-error)
🪛 GitHub Check: SonarCloud Code Analysis
src/runtime/core/argumentparser.h
[warning] 134-134: Replace this const reference to "std::string" by a "std::string_view".
src/runtime/core/argumentparser.cpp
[warning] 25-25: Define each identifier in a dedicated statement.
[warning] 186-186: Replace this const reference to "std::string" by a "std::string_view".
[warning] 56-56: Use the init-statement to declare "error" inside the if statement.
[failure] 51-51: Refactor this function to reduce its Cognitive Complexity from 43 to the 25 allowed.
[warning] 129-129: Use "std::format" or "std::format_to_n" instead of "sprintf". Use "std::format_to" only if buffer size is guaranteed to be sufficient.
[warning] 223-223: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
[failure] 283-283: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 68-68: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[warning] 158-158: Replace this const reference to "std::string" by a "std::string_view".
[warning] 52-52: Do not use the constructor's initializer list for data member "mCurrentIndex". Use the in-class initializer instead.
[warning] 170-170: Replace this const reference to "std::string" by a "std::string_view".
[warning] 281-281: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
[warning] 52-52: Do not use the constructor's initializer list for data member "mIsValid". Use the in-class initializer instead.
[warning] 201-201: Replace this const reference to "std::string" by a "std::string_view".
[warning] 66-66: Use the init-statement to declare "pos" inside the if statement.
[warning] 127-127: Use "std::string" instead of a C-style char array.
src/runtime/core/tokenizer.cpp
[warning] 38-38: Use the init-statement to declare "tmp" inside the if statement.
[warning] 29-29: Replace this const reference to "std::string" by a "std::string_view".
[warning] 29-29: Replace this const reference to "std::string" by a "std::string_view".
🔇 Additional comments (4)
src/runtime/core/tokenizer.cpp (4)
9-11: LGTM!The implementation correctly returns the token count.
21-23: LGTM!The implementation correctly delegates to the underlying container.
25-27: LGTM!The implementation correctly clears the token container.
52-60: LGTM!The implementation correctly delegates to the static tokenize method. Note that line 54 uses
resize(0)which is functionally equivalent toclear()that's called elsewhere.
| size_t Tokenizer::tokenize(const std::string &str, StringArray &tokens, const std::string &delimiters) { | ||
| // Skip delimiters at beginning. | ||
| std::string::size_type lastPos = str.find_first_not_of( delimiters, 0 ); | ||
|
|
||
| // find first "non-delimiter". | ||
| std::string::size_type pos = str.find_first_of( delimiters, lastPos ); | ||
| while( std::string::npos != pos || std::string::npos != lastPos ) { | ||
| // Found a token, add it to the vector. | ||
| std::string tmp = str.substr( lastPos, pos - lastPos ); | ||
| if ( !tmp.empty() && ' ' != tmp[ 0 ] ) { | ||
| tokens.add( tmp ); | ||
| } | ||
|
|
||
| // Skip delimiters. Note the "not_of" | ||
| lastPos = str.find_first_not_of( delimiters, pos ); | ||
|
|
||
| // Find next "non-delimiter" | ||
| pos = str.find_first_of( delimiters, lastPos ); | ||
| } | ||
|
|
||
| return static_cast<uint32_t>( tokens.size() ); | ||
| } |
There was a problem hiding this comment.
Verify token filtering logic and fix return type inconsistency.
The function has the following concerns:
-
Line 49: Returns
static_cast<uint32_t>(tokens.size())when the function signature specifiessize_t. This narrowing conversion could lose data if the token count exceeds UINT32_MAX. -
Line 38: The condition
' ' != tmp[0]filters out tokens starting with a space. Ensure this behavior is intentional for your use case, as general tokenizers typically preserve all tokens between delimiters.
Apply this diff to fix the return type:
- return static_cast<uint32_t>( tokens.size() );
+ return tokens.size();Additionally, consider the SonarCloud suggestions:
- Use
std::string_viewfor thestranddelimitersparameters to avoid unnecessary copies. - Move the
tmpvariable declaration into the if statement scope at line 38.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 38-38: Use the init-statement to declare "tmp" inside the if statement.
[warning] 29-29: Replace this const reference to "std::string" by a "std::string_view".
[warning] 29-29: Replace this const reference to "std::string" by a "std::string_view".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/runtime/core/argumentparser.cpp (2)
46-94: Fix potential out-of-bounds access when validating option values in the constructorWhen an option expects
numValues > 0, theforloop overvalIdxdoes not check againstargc. If the user provides fewer tokens thannumValues,ppArgv[valIdx]is read pastargc - 1, which is undefined behaviour and can crash. This is the same issue raised earlier and still present here.You can guard with an unsigned copy of
argcand validate the remaining token count before iterating:- if (argc > 1) { - while (mCurrentIndex < static_cast<uint32_t>(argc)) { + const uint32_t argcU = static_cast<uint32_t>(argc); + if (argcU > 1) { + while (mCurrentIndex < argcU) { std::string argument(ppArgv[mCurrentIndex]); const std::string::size_type pos = argument.find(option); if (std::string::npos != pos) { argument = argument.substr(pos + optionLen, argument.size() - (pos + optionLen)); if (isSupported(argument)) { if (const uint32_t numValues = getNumValues(argument)) { - // Check the number of expected values for being a new option - for (uint32_t valIdx = mCurrentIndex + 1; valIdx < mCurrentIndex + numValues + 1; valIdx++) { - std::string tmpVal(ppArgv[valIdx]); - std::string::size_type pos1 = tmpVal.find(option); - if (pos1 != std::string::npos) { - setInvalid(); - break; - } - } + // Ensure there are enough values and none of them is a new option + if (mCurrentIndex + numValues >= argcU) { + setInvalid(); + } else { + for (uint32_t valIdx = mCurrentIndex + 1; + valIdx <= mCurrentIndex + numValues; + ++valIdx) { + std::string tmpVal(ppArgv[valIdx]); + const std::string::size_type pos1 = tmpVal.find(option); + if (pos1 != std::string::npos) { + setInvalid(); + break; + } + } + } }This preserves the original intent (detecting unexpected new options) while avoiding any access beyond
ppArgv[argcU - 1].
181-194: Static Dummy string in getArgument is not thread-safe and hides “not found” vs “empty”
getArgumentreturns a reference to a single staticstd::string Dummy. Multiple threads calling this concurrently can race onDummy, and callers cannot distinguish “argument not present” from “present with empty value”. This is the same concern raised previously and still unresolved.Two options:
- Minimal change (keep API, fix data race):
const std::string &ArgumentParser::getArgument(const std::string &arg) const { - static std::string Dummy; + thread_local std::string Dummy;
- Better API (if you can change the signature): change the return type to
std::optional<std::string>in the header and implementation, returnstd::nulloptwhen the arg is empty/not found, and a value copy when found.Either way, consider documenting the behaviour so callers can reliably distinguish missing arguments.
🧹 Nitpick comments (1)
src/runtime/core/argumentparser.cpp (1)
22-41: parseExpectedArgs silently ignores invalid argument definitions from parseArgParameterIf
ArgumentParser::parseArgParameter(arg, numParam)returnsfalse, the current code just skips adding that argument and still returnstruewith an empty error. That makes misconfigured argument specs fail silently and leaveserrorunused.Consider treating a failed parse as a hard error and populating
error:- for (uint32_t i = 0; i < extractedArgs.size(); ++i) { - std::string arg = extractedArgs[i]; - std::string currentDesc = extractedDescs[i]; - if (ArgumentParser::parseArgParameter(arg, numParam)) { - supportedArgs.add(ArgumentParser::Argument(ArgumentParser::getBlankArgument(arg), - currentDesc, numParam)); - } - } + for (uint32_t i = 0; i < extractedArgs.size(); ++i) { + std::string arg = extractedArgs[i]; + std::string currentDesc = extractedDescs[i]; + if (!ArgumentParser::parseArgParameter(arg, numParam)) { + error += "Invalid argument definition: " + arg; + return false; + } + + supportedArgs.add(ArgumentParser::Argument( + ArgumentParser::getBlankArgument(arg), + currentDesc, + numParam)); + }This way configuration mistakes are detected early and reported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/runtime/core/argumentparser.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/core/argumentparser.cpp (2)
src/runtime/core/tokenizer.cpp (6)
tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)clear(25-27)clear(25-25)src/runtime/core/argumentparser.h (2)
string(56-136)string(138-148)
🪛 Clang (14.0.6)
src/runtime/core/argumentparser.cpp
[error] 1-1: 'core/argumentparser.h' file not found
(clang-diagnostic-error)
🪛 GitHub Check: SonarCloud Code Analysis
src/runtime/core/argumentparser.cpp
[warning] 25-25: Define each identifier in a dedicated statement.
[warning] 276-276: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
[warning] 124-124: Use "std::format" or "std::format_to_n" instead of "sprintf". Use "std::format_to" only if buffer size is guaranteed to be sufficient.
[warning] 181-181: Replace this const reference to "std::string" by a "std::string_view".
[warning] 165-165: Replace this const reference to "std::string" by a "std::string_view".
[warning] 51-51: Use the init-statement to declare "error" inside the if statement.
[warning] 196-196: Replace this const reference to "std::string" by a "std::string_view".
[failure] 278-278: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 46-46: Refactor this function to reduce its Cognitive Complexity from 43 to the 25 allowed.
[warning] 47-47: Do not use the constructor's initializer list for data member "mCurrentIndex". Use the in-class initializer instead.
[warning] 61-61: Use the init-statement to declare "pos" inside the if statement.
[warning] 122-122: Use "std::string" instead of a C-style char array.
[warning] 153-153: Replace this const reference to "std::string" by a "std::string_view".
[failure] 63-63: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[warning] 47-47: Do not use the constructor's initializer list for data member "mIsValid". Use the in-class initializer instead.
[warning] 218-218: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (ubuntu-latest, Release, clang)
| bool ArgumentParser::getNext(std::string &arg, std::string &value) { | ||
| if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || | ||
| mCurrentIndex >= mSupportedArguments.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| arg = mSupportedArguments[mCurrentIndex].mArgument; | ||
| value = mStoredArguments[mCurrentIndex]; | ||
| ++mCurrentIndex; | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
getNext mixes supported and detected arguments and can read past mStoredArguments
getNext bounds mCurrentIndex by mSupportedArguments.size() but indexes into mStoredArguments. If the number of supported arguments exceeds the number of detected arguments (very likely), a second or later call to getNext can access mStoredArguments[mCurrentIndex] out-of-bounds, and argument names/values are not correctly paired.
You probably want to iterate over the detected arguments instead:
bool ArgumentParser::getNext(std::string &arg, std::string &value) {
- if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() ||
- mCurrentIndex >= mSupportedArguments.size()) {
+ if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() ||
+ mCurrentIndex >= mDetectedArgs.size()) {
return false;
}
- arg = mSupportedArguments[mCurrentIndex].mArgument;
+ arg = mDetectedArgs[mCurrentIndex];
value = mStoredArguments[mCurrentIndex];
++mCurrentIndex;
return true;
}This keeps arg and value aligned with what was actually parsed and prevents out-of-bounds access on mStoredArguments.
🤖 Prompt for AI Agents
In src/runtime/core/argumentparser.cpp around lines 132-143, getNext currently
bounds mCurrentIndex against mSupportedArguments.size() but indexes into
mStoredArguments, which can cause out-of-bounds access and mispaired
names/values; change the logic to iterate over the detected arguments instead by
using mStoredArguments.size() as the bound, and when setting arg try to retrieve
the matching supported argument (if mCurrentIndex < mSupportedArguments.size()
use mSupportedArguments[mCurrentIndex].mArgument, otherwise fall back to a safe
default such as an empty string or the stored key), increment mCurrentIndex and
return true; ensure the initial emptiness check uses mStoredArguments.isEmpty()
and that mCurrentIndex is compared to mStoredArguments.size() to prevent reading
past the parsed values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/runtime/core/argumentparser.cpp (2)
139-150: Critical:getNextstill mixes supported and detected arguments (unresolved).This function bounds-checks against
mSupportedArguments.size()but reads frommStoredArguments, and returns the wrong argument names. If you have 10 supported arguments but only 2 detected, this breaks after the second call. More importantly, it returns supported argument names instead of detected ones, mispairing names with values.Apply this diff to iterate over detected arguments:
bool ArgumentParser::getNext(std::string &arg, std::string &value) { - if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || - mCurrentIndex >= mSupportedArguments.size()) { + if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() || + mCurrentIndex >= mDetectedArgs.size()) { return false; } - arg = mSupportedArguments[mCurrentIndex].mArgument; + arg = mDetectedArgs[mCurrentIndex]; value = mStoredArguments[mCurrentIndex]; ++mCurrentIndex; return true; }
188-201: Major: StaticDummyvariable is not thread-safe (unresolved).The
static std::string Dummyis shared across all calls and threads, creating a race condition. Additionally, callers cannot distinguish between "argument not found" and "argument found with empty value."Consider returning
std::optional<std::string>if C++17 is available:#include <optional> std::optional<std::string> ArgumentParser::getArgument(const std::string &arg) const { if (arg.empty()) { return std::nullopt; } for (uint32_t index = 0; index < mDetectedArgs.size(); ++index) { if (mDetectedArgs[index] == arg) { return mStoredArguments[index]; } } return std::nullopt; }Otherwise, make
Dummythread-local:const std::string &ArgumentParser::getArgument(const std::string &arg) const { - static std::string Dummy; + thread_local std::string Dummy;
🧹 Nitpick comments (1)
src/runtime/core/argumentparser.cpp (1)
127-133: Replace unsafesprintfwithstd::to_string.Using
sprintfwith a fixed buffer risks overflow and is not idiomatic modern C++. Since you're only formatting a single integer,std::to_stringis simpler and safer.Apply this diff:
if (numargs > 0) { helpMsg += "\n\tNumber of arguments: "; - char buffer[512]; - memset(buffer, '\0', 512); - sprintf(buffer, "%d\n", numargs); - helpMsg += std::string(buffer); + helpMsg += std::to_string(numargs) + "\n"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/runtime/core/argumentparser.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/core/argumentparser.cpp (2)
src/runtime/core/tokenizer.cpp (6)
tokenize(29-50)tokenize(29-29)tokenize(52-60)tokenize(52-52)clear(25-27)clear(25-25)src/runtime/core/argumentparser.h (2)
string(56-136)string(138-148)
🪛 Clang (14.0.6)
src/runtime/core/argumentparser.cpp
[error] 1-1: 'core/argumentparser.h' file not found
(clang-diagnostic-error)
🪛 GitHub Check: SonarCloud Code Analysis
src/runtime/core/argumentparser.cpp
[warning] 283-283: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
[warning] 25-25: Define each identifier in a dedicated statement.
[warning] 188-188: Replace this const reference to "std::string" by a "std::string_view".
[warning] 203-203: Replace this const reference to "std::string" by a "std::string_view".
[warning] 47-47: Do not use the constructor's initializer list for data member "mCurrentIndex". Use the in-class initializer instead.
[failure] 46-46: Refactor this function to reduce its Cognitive Complexity from 52 to the 25 allowed.
[failure] 285-285: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[warning] 51-51: Use the init-statement to declare "error" inside the if statement.
[warning] 47-47: Do not use the constructor's initializer list for data member "mIsValid". Use the in-class initializer instead.
[warning] 62-62: Use the init-statement to declare "pos" inside the if statement.
[failure] 64-64: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[warning] 172-172: Replace this const reference to "std::string" by a "std::string_view".
[warning] 131-131: Use "std::format" or "std::format_to_n" instead of "sprintf". Use "std::format_to" only if buffer size is guaranteed to be sufficient.
[warning] 57-57: Replace the redundant type with "auto".
[warning] 225-225: Change this raw for-loop to a range for-loop or an "std::ranges::for_each".
[warning] 129-129: Use "std::string" instead of a C-style char array.
[warning] 160-160: Replace this const reference to "std::string" by a "std::string_view".
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-latest, Release, gcc)
- GitHub Check: build (ubuntu-latest, Release, clang)
🔇 Additional comments (1)
src/runtime/core/argumentparser.cpp (1)
22-44: LGTM! Past issue resolved.The premature size check has been correctly removed. The function now tokenizes first, then validates the sizes, which is the proper order.
| if (mIsValid) { | ||
| if ((mCurrentIndex + 1) < static_cast<uint32_t>(argc)) { | ||
| ++mCurrentIndex; | ||
| mDetectedArgs.add(argument); | ||
| std::string content(ppArgv[mCurrentIndex]); | ||
| mStoredArguments.add(content); | ||
| } else { | ||
| mDetectedArgs.add(argument); | ||
| mStoredArguments.add(""); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Only one value is stored for multi-value arguments.
When numValues > 1, the validation loop (lines 65-81) correctly checks that multiple values are present, but the storage logic (lines 84-94) only stores the first value. Arguments expecting multiple values will lose all but the first.
Apply this diff to store all expected values:
// Store the data if its valid
if (mIsValid) {
- if ((mCurrentIndex + 1) < static_cast<uint32_t>(argc)) {
- ++mCurrentIndex;
- mDetectedArgs.add(argument);
- std::string content(ppArgv[mCurrentIndex]);
- mStoredArguments.add(content);
- } else {
- mDetectedArgs.add(argument);
- mStoredArguments.add("");
+ mDetectedArgs.add(argument);
+
+ const uint32_t numValues = getNumValues(argument);
+ if (numValues > 0) {
+ std::string combinedValues;
+ for (uint32_t i = 0; i < numValues && (mCurrentIndex + 1 + i) < argcU; ++i) {
+ if (i > 0) combinedValues += " ";
+ combinedValues += ppArgv[mCurrentIndex + 1 + i];
+ }
+ mStoredArguments.add(combinedValues);
+ mCurrentIndex += numValues;
+ } else {
+ if ((mCurrentIndex + 1) < argcU) {
+ ++mCurrentIndex;
+ mStoredArguments.add(ppArgv[mCurrentIndex]);
+ } else {
+ mStoredArguments.add("");
+ }
}
}🤖 Prompt for AI Agents
In src/runtime/core/argumentparser.cpp around lines 84-94: the code currently
only stores a single value even when an argument expects multiple values;
replace the single-value store with logic that, after
mDetectedArgs.add(argument), loops for the argument's expected numValues and for
each iteration increments mCurrentIndex, reads ppArgv[mCurrentIndex] (or uses an
empty string if out of range), adds that string to mStoredArguments, and ensures
mCurrentIndex is advanced for every consumed value so all expected values are
stored (single-value arguments still consume exactly one entry).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/application/app.cpp (1)
30-35: Duplicate function definition will cause compilation error.A second
getEndLog()function is defined with the same signature as the one at lines 23-28. This creates a redefinition error and will prevent the code from compiling.Remove one of the duplicate definitions. Based on the formatting consistency with
getStartLog(), the first definition (lines 23-28) appears to be the intended one.🔎 Proposed fix
- std::string getEndLog() { - std::string entry = "===========================================================================\n"; - entry.append(" SegFault run ended\n"); - entry.append("==========================================================================="); - return entry; - }
♻️ Duplicate comments (1)
src/runtime/application/app.cpp (1)
18-18: Version string contains a typo (letter 'l' instead of digit).The version string
"0.0.l"uses a lowercase letter 'l' instead of a digit. This should likely be"0.0.1"or the correct numeric version.
🧹 Nitpick comments (1)
src/runtime/renderer/rendercore.h (1)
24-26: Empty placeholder struct should be completed or documented.The
RenderPipelineStatestruct is currently empty and not actively used (its usage inDrawCommandat line 28 is commented out). This suggests incomplete work-in-progress.Consider either:
- Implementing the struct with the necessary state fields (shader program, blend state, depth/stencil settings, etc.)
- Adding a TODO comment to track future implementation
- Removing it if not yet needed
Would you like me to help outline typical render pipeline state fields based on common rendering APIs?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/runtime/application/app.cppsrc/runtime/renderer/RHIVulkan.cppsrc/runtime/renderer/rendercore.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/renderer/RHIVulkan.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/application/app.cpp (1)
src/runtime/core/segfault.h (1)
logMessage(62-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest, Release, cl)
🔇 Additional comments (1)
src/runtime/application/app.cpp (1)
133-136: Good defensive null check in shutdown.The early return when
mSdlWindowis null prevents potential crashes from callingSDL_DestroyWindowon a null pointer. This is good defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/application/app.cpp (1)
125-136: Memory leak:mRHIallocated but never cleaned up in shutdown.
mRHIis allocated withnewat line 93 but neither itsshutdown()method is called nor the object is deleted inshutdown(). SinceRHIprovides a publicshutdown()method, it must be called before deletion to allow proper cleanup of its internal state (includingmImpl).Add the following cleanup before SDL teardown:
void App::shutdown() { if (mSdlWindow == nullptr) { logMessage(LogType::Error, "Invalid application state, cannot shutdown."); return; } + + if (mRHI != nullptr) { + mRHI->shutdown(); + delete mRHI; + mRHI = nullptr; + } SDL_DestroyWindow(mSdlWindow); mSdlWindow = nullptr; mState = ModuleState::Shutdown; SDL_Quit(); logMessage(LogType::Print, getEndLog().c_str()); }
♻️ Duplicate comments (1)
src/runtime/application/app.cpp (1)
18-18: Version string typo:0.0.lshould likely be0.0.1The version string uses the letter
linstead of the digit1. This appears to be a typo.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/contrib/cppcoresrc/runtime/application/app.cpp
✅ Files skipped from review due to trivial changes (1)
- src/contrib/cppcore
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/application/app.cpp (1)
src/runtime/core/segfault.h (1)
logMessage(62-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-latest, Release, gcc)
- GitHub Check: build (ubuntu-latest, Release, clang)
🔇 Additional comments (1)
src/runtime/application/app.cpp (1)
23-28: LGTM!The end log banner formatting is consistent with the start log banner style.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/runtime/core/argumentparser.cpp (4)
48-51: Remove dead code: size check before tokenization.Lines 48-51 check
extractedDescs.size() != extractedArgs.size()immediately after declaring both arrays empty. This condition is always false (0 == 0) and has no effect. The actual validation correctly happens at lines 56-59 after tokenization.
111-121: Critical: Only one value stored for multi-value arguments.When an argument expects
numValues > 1, the validation loop (lines 92-108) correctly checks that multiple values are present, but the storage logic (lines 111-121) only stores the first value and doesn't advancemCurrentIndexby the number of consumed values. Arguments expecting multiple values will lose all but the first.Store all expected values and advance the index accordingly:
🔎 Proposed fix
// Store the data if its valid if (mIsValid) { + mDetectedArgs.add(argument); + + const uint32_t numValues = getNumValues(argument); + if (numValues > 0) { + std::string combinedValues; + for (uint32_t i = 0; i < numValues && (mCurrentIndex + 1 + i) < argcU; ++i) { + if (i > 0) combinedValues += " "; + combinedValues += ppArgv[mCurrentIndex + 1 + i]; + } + mStoredArguments.add(combinedValues); + mCurrentIndex += numValues; + } else { - if ((mCurrentIndex + 1) < static_cast<uint32_t>(argc)) { - ++mCurrentIndex; - mDetectedArgs.add(argument); - std::string content(ppArgv[mCurrentIndex]); - mStoredArguments.add(content); - } else { - mDetectedArgs.add(argument); - mStoredArguments.add(""); + if ((mCurrentIndex + 1) < argcU) { + ++mCurrentIndex; + mStoredArguments.add(ppArgv[mCurrentIndex]); + } else { + mStoredArguments.add(""); + } } }
166-177: Critical:getNextmixes supported and detected arguments, causing out-of-bounds access.
getNextboundsmCurrentIndexbymSupportedArguments.size()but indexes intomStoredArguments. If the number of supported arguments exceeds the number of detected arguments (very likely), subsequent calls togetNextwill accessmStoredArguments[mCurrentIndex]out-of-bounds. Additionally, argument names and values will not be correctly paired.Iterate over detected arguments instead:
🔎 Proposed fix
bool ArgumentParser::getNext(std::string &arg, std::string &value) { - if (mSupportedArguments.isEmpty() || mStoredArguments.isEmpty() || - mCurrentIndex >= mSupportedArguments.size()) { + if (mDetectedArgs.isEmpty() || mStoredArguments.isEmpty() || + mCurrentIndex >= mDetectedArgs.size()) { return false; } - arg = mSupportedArguments[mCurrentIndex].mArgument; + arg = mDetectedArgs[mCurrentIndex]; value = mStoredArguments[mCurrentIndex]; ++mCurrentIndex; return true; }
215-228: StaticDummyvariable is not thread-safe.The
static std::string Dummyat line 216 is shared across all calls togetArgument. If multiple threads call this method, there's a potential race condition. Additionally, callers cannot distinguish between an argument with an empty value and an argument that was not found.Consider one of these alternatives:
Option 1: Return
std::optional<std::string>(requires C++17):🔎 Proposed fix using std::optional
+#include <optional> + -const std::string &ArgumentParser::getArgument(const std::string &arg) const { - static std::string Dummy; +std::optional<std::string> ArgumentParser::getArgument(const std::string &arg) const { if (arg.empty()) { - return Dummy; + return std::nullopt; } for (uint32_t index = 0; index < mDetectedArgs.size(); ++index) { if (mDetectedArgs[index] == arg) { return mStoredArguments[index]; } } - return Dummy; + return std::nullopt; }Update the header declaration accordingly.
Option 2: Use
thread_localif C++11:🔎 Alternative fix using thread_local
const std::string &ArgumentParser::getArgument(const std::string &arg) const { - static std::string Dummy; + thread_local std::string Dummy; if (arg.empty()) { return Dummy; }
🧹 Nitpick comments (2)
src/runtime/core/argumentparser.cpp (2)
156-159: Consider usingstd::to_stringinstead ofsprintf.The
sprintfusage here is safe but could be replaced with the modern C++std::to_string(numargs)for clearer intent and to avoid manual buffer management.🔎 Proposed refactor
if (numargs > 0) { helpMsg += "\n\tNumber of arguments: "; - char buffer[512]; - memset(buffer, '\0', 512); - sprintf(buffer, "%d\n", numargs); - helpMsg += std::string(buffer); + helpMsg += std::to_string(numargs); + helpMsg += "\n"; }
272-272: Consider usingstd::stoiinstead ofatoifor better error handling.
atoireturns 0 on error or if the input is "0", making it impossible to distinguish between valid zero and parse failure.std::stoiwith exception handling would be more robust.🔎 Proposed refactor
if (!paraDefOpen) { - numPara = atoi(paraStr.c_str()); + try { + numPara = static_cast<uint32_t>(std::stoi(paraStr)); + } catch (const std::exception&) { + numPara = 0; + return false; + } } else { return false; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/contrib/cppcoresrc/runtime/application/app.cppsrc/runtime/core/argumentparser.cppsrc/runtime/core/argumentparser.hsrc/runtime/renderer/RHIVulkan.cpp
💤 Files with no reviewable changes (1)
- src/runtime/renderer/RHIVulkan.cpp
✅ Files skipped from review due to trivial changes (1)
- src/contrib/cppcore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/application/app.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/core/argumentparser.h (1)
src/runtime/core/argumentparser.cpp (26)
ArgumentParser(73-135)Argument(34-37)Argument(39-42)showHelp(137-164)showHelp(137-137)getNext(166-177)getNext(166-166)isSupported(187-197)isSupported(187-187)getNumValues(199-213)getNumValues(199-199)argument(87-87)getArgument(215-228)getArgument(215-215)hasArgument(230-239)hasArgument(230-230)hasValidArgs(241-243)hasValidArgs(241-241)parseArgParameter(245-278)parseArgParameter(245-245)getBlankArgument(280-301)getBlankArgument(280-280)validateArguments(303-326)validateArguments(303-303)setInvalid(328-330)setInvalid(328-328)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-latest, Release, gcc)
- GitHub Check: build (ubuntu-latest, Release, clang)
🔇 Additional comments (1)
src/runtime/core/argumentparser.h (1)
1-172: Header declarations look sound.The header file correctly declares the ArgumentParser API. However, there are critical implementation issues in the corresponding .cpp file that need to be addressed (see comments on argumentparser.cpp).
|
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.