ncnn2mem support new style array and string type #6440
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances the ncnn2mem tool to support new style arrays (comma-separated values like 1,2,3) and string type parameters in addition to the existing old style arrays (length-prefixed like -23300=3,1,2,3).
Key changes:
- Added
vstr_is_string()function to detect string parameters - Refactored parameter parsing to handle three types: old style arrays (id <= -23300), strings (-id - 23400), and new style comma-separated arrays (-id - 23300)
- Implemented string parsing with support for quoted and unquoted strings
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char end = vstr[len - 1]; | ||
| if (end != '\"') | ||
| { | ||
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | ||
| } | ||
| else | ||
| nscan = 0; // already ended with a quote, no need to scan more |
There was a problem hiding this comment.
The len calculation can cause undefined behavior when vstr is empty or only contains a null terminator. If vstr[0] == '\"' and the string has length 1 (only a quote), then len would be 1 and accessing vstr[len - 1] (i.e., vstr[0]) would incorrectly check the opening quote instead of detecting there's no closing quote.
Consider checking if len > 1 before accessing vstr[len - 1] to avoid edge cases:
int len = 0;
while (vstr[len] != '\0')
len++;
if (len > 1)
{
char end = vstr[len - 1];
if (end != '\"')
{
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
nscan = 0;
}
else if (len == 1)
{
// Only opening quote, need to read the rest
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}| char end = vstr[len - 1]; | |
| if (end != '\"') | |
| { | |
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | |
| } | |
| else | |
| nscan = 0; // already ended with a quote, no need to scan more | |
| if (len > 1) | |
| { | |
| char end = vstr[len - 1]; | |
| if (end != '\"') | |
| { | |
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | |
| } | |
| else | |
| nscan = 0; // already ended with a quote, no need to scan more | |
| } | |
| else if (len == 1) | |
| { | |
| // Only opening quote, need to read the rest | |
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | |
| } | |
| else | |
| { | |
| // Empty string, nothing to scan | |
| nscan = 0; | |
| } |
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | ||
| } | ||
| else | ||
| nscan = 0; // already ended with a quote, no need to scan more | ||
| } | ||
| else | ||
| { | ||
| nscan = fscanf(fp, "%255[^\n ]", vstr2); |
There was a problem hiding this comment.
The format string "%255[^\n ]" can read up to 255 characters into vstr2 (size 256), which is correct. However, the subsequent validation at line 373 checks vstr2[241] instead of checking if the buffer was fully utilized. This inconsistency means strings between 241 and 255 characters would pass validation even though they exceed the intended limit (15 from vstr + 240 from vstr2 = 255 max).
Either adjust the format to "%240[^\n ]" to enforce the 255 total limit, or fix the validation to check the appropriate position.
| nscan = fscanf(fp, "%255[^\"\n]\"", vstr2); | |
| } | |
| else | |
| nscan = 0; // already ended with a quote, no need to scan more | |
| } | |
| else | |
| { | |
| nscan = fscanf(fp, "%255[^\n ]", vstr2); | |
| nscan = fscanf(fp, "%240[^\"\n]\"", vstr2); | |
| } | |
| else | |
| nscan = 0; // already ended with a quote, no need to scan more | |
| } | |
| else | |
| { | |
| nscan = fscanf(fp, "%240[^\n ]", vstr2); |
| str = std::string(vstr); | ||
| } | ||
|
|
||
| if (str[str.size() - 1] == '\"') |
There was a problem hiding this comment.
Accessing str[str.size() - 1] without first checking if str is empty can cause undefined behavior. If the string is empty (which could happen if vstr[0] == '\"' and vstr[1] == '\0'), then str.size() would be 0, and str.size() - 1 would wrap around to a very large number due to unsigned arithmetic.
Add a check before accessing the last character:
if (!str.empty() && str[str.size() - 1] == '\"')
str.resize(str.size() - 1);| if (str[str.size() - 1] == '\"') | |
| if (!str.empty() && str[str.size() - 1] == '\"') |
|
|
||
| // scan the remaining string | ||
| char vstr2[256]; | ||
| vstr2[241] = '\0'; // max 255 = 15 + 240 |
There was a problem hiding this comment.
The magic number 241 and the comment explaining "max 255 = 15 + 240" suggest a constraint, but using 241 as a sentinel is confusing. Consider using a named constant to clarify the intent:
const int MAX_VSTR2_LEN = 240;
char vstr2[MAX_VSTR2_LEN + 1];
vstr2[MAX_VSTR2_LEN] = '\0';Then check vstr2[MAX_VSTR2_LEN] != '\0' at line 373. This makes the relationship between the sizes explicit.
|
|
||
| bool is_string = vstr_is_string(vstr); | ||
| if (is_string) | ||
| { |
There was a problem hiding this comment.
The transformation id = -id - 23400 is a magic number calculation. Consider adding a comment explaining that 23400 is the encoding offset for string types, similar to how 23300 is used for arrays. This would improve code readability and maintainability.
| { | |
| { | |
| // 23400 is the encoding offset for string types (similar to 23300 for arrays) |
|
The binary size change of libncnn.so (bytes)
|
2 participants
No description provided.