Adding back private implementation of printf #3026
Conversation
WalkthroughThe changes introduce the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (41)
src/CLR/Diagnostics/Diagnostics.h (1)
Line range hint
7-13: Consider removing commented-out includes if no longer needed.There are several commented-out include statements in this file:
// #include <nanoCLR_Diagnostics.h> // #include <nanoCLR_Graphics.h> // #include <nanoCLR_Hardware.h> // #include <core.h> // #include <spot_net_native.h>If these includes are no longer needed, it would be best to remove them entirely rather than leaving them commented out. This helps maintain cleaner and more maintainable code. If they are still needed but currently disabled for a specific reason, please add a comment explaining why they are commented out and under what circumstances they might be needed in the future.
Also applies to: 17-17
src/CLR/Diagnostics/Info_Safeprintf.cpp (1)
Line range hint
20-24: LGTM: Overall changes are well-implementedThe modifications to
CLR_SafeSprintfVare well-implemented and improve its robustness. The unchanged parts, including buffer null-termination and pointer/size updates, remain correct and consistent with the changes.Minor optimization suggestion: Consider combining the buffer update operations into a single statement for slightly improved readability:
iBuffer -= (szBuffer += chars)[0] = 0;This change is optional and doesn't affect functionality.
The
CLR_SafeSprintffunction remains unchanged and correct.Also applies to: 26-41
src/CLR/Core/Core.vcxproj.filters (1)
135-137: LGTM! Consider grouping related files together.The addition of
nanoprintf.cto the project is correct and aligns with the PR objectives. The file is appropriately placed in the "Source Files" filter.For better organization, consider grouping related files together. You might want to move this entry next to other string or formatting-related source files if they exist in the project.
src/CLR/CorLib/CorLib.vcxproj (2)
155-155: Please clarify the rationale for using a private printf implementation.The addition of the nanoprintf library aligns with the PR objective of "Adding back private implementation of printf". While this approach can offer benefits such as increased control, potential performance improvements, and customization options, it's important to consider the long-term implications:
- Maintenance: How will the project ensure the private implementation stays up-to-date and bug-free?
- Compatibility: Are there any known deviations from standard printf behavior that developers should be aware of?
- Performance: Has the team conducted benchmarks to confirm performance improvements over the previous implementation?
Could you please provide more context on why this private implementation is preferred over standard library options?
Also applies to: 168-168, 183-183, 200-200
211-211: Minor: Newline character removed at end of file.The newline character at the end of the file has been removed. While this doesn't affect functionality, it's worth noting that some coding standards recommend keeping a newline at the end of files. Consider checking your project's style guide for any specific requirements.
Overall, the changes in this file are minimal and focused on integrating the nanoprintf library. The consistency across different build configurations is commendable, and the changes align well with the stated PR objectives.
targets/ChibiOS/_nanoCLR/nanoCRT.cpp (7)
Line range hint
15-42: Leveragenanoprintffor floating-point formatting inhal_snprintf_floatThe
hal_snprintf_floatfunction implements custom floating-point formatting logic. Since thenanoprintflibrary supports floating-point formatting, consider utilizingnpf_snprintfto simplify the code and maintain consistency.Refactor the function to use
npf_snprintffrom the nanoprintf library:-int hal_snprintf_float(char *buffer, size_t len, const char *format, int32_t f) -{ - // Custom implementation -} +int hal_snprintf_float(char *buffer, size_t len, const char *format, float f) +{ + return npf_snprintf(buffer, len, format, f); +}
Line range hint
44-73: Simplifyhal_snprintf_doubleby usingnanoprintfThe
hal_snprintf_doublefunction manually handles double formatting. To reduce complexity and potential errors, consider usingnpf_snprintffrom the nanoprintf library, which already provides robust support for double formatting.Modify the function as follows:
-int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t &d) -{ - // Custom implementation -} +int hal_snprintf_double(char *buffer, size_t len, const char *format, double d) +{ + return npf_snprintf(buffer, len, format, d); +}
Line range hint
83-94: Ensure buffer safety indebug_printfThe
debug_printffunction uses a fixed-size bufferchar buffer[256];without ensuring the formatted output fits within the buffer, risking a buffer overflow.Implement a check to ensure the formatted string does not exceed the buffer size. You can adjust the
vsnprintfcall and handle cases where the output is truncated:- int len = vsnprintf(buffer, sizeof(buffer) - 1, format, arg_ptr); + int len = vsnprintf(buffer, sizeof(buffer), format, arg_ptr); + if (len < 0) + { + // Handle encoding errors + len = 0; + } + else if ((size_t)len >= sizeof(buffer)) + { + // Output was truncated to fit the buffer + len = sizeof(buffer) - 1; + } + buffer[len] = '\0'; // Ensure null termination
Line range hint
96-117: Use safer string copying inhal_strcpy_sThe use of
strcpyposes a risk of buffer overflows despite prior size checks. Employstrncpyor ensure that the destination buffer is adequately handled to prevent potential security issues.Update the function to use
strncpyand guarantee null termination:- strcpy(strDst, strSrc); + strncpy(strDst, strSrc, sizeInBytes - 1); + strDst[sizeInBytes - 1] = '\0'; // Ensure null termination
Line range hint
119-141: Prevent potential out-of-bounds write inhal_strncpy_sAssigning
strDst[count] = 0;may result in writing beyond the buffer ifcountequals or exceedssizeInBytes.Modify the function to include a check and adjust the null termination accordingly:
if (sizeInBytes < count + 1) { _ASSERTE(FALSE); strDst[0] = 0; return 1; } - strDst[count] = 0; strncpy(strDst, strSrc, count); + strDst[count] = 0; // Ensure null termination within bounds return 0;
Line range hint
143-152: Add null pointer check inhal_strlen_sThe function
hal_strlen_sdoes not check ifstrisNULLbefore dereferencing it, which can lead to undefined behavior.Include a null pointer check at the beginning of the function:
+ if (str == NULL) + { + _ASSERTE(FALSE); + return 0; + }
Line range hint
154-166: Clarify return values inhal_strncmp_sfor invalid inputsCurrently, when either
str1orstr2isNULL, the function returns1, which could be confused with a valid comparison result.Return a distinct error code to indicate invalid input parameters:
if (str1 == NULL || str2 == NULL) { _ASSERTE(FALSE); - return 1; + return INT_MAX; // Return a distinct error code }targets/FreeRTOS/_common/nanoCLR/nanoCRT.cpp (6)
Line range hint
25-55: Refactor duplicate logic inhal_snprintf_floatandhal_snprintf_doubleThe functions
hal_snprintf_floatandhal_snprintf_doubleshare similar logic for formatting numbers. Consider refactoring the common code into a helper function to reduce duplication and improve maintainability.
Line range hint
56-91: Unnecessary passing of parameter by reference inhal_snprintf_doubleThe parameter
dinhal_snprintf_doubleis passed by reference (int64_t &d), but the function does not modify it. Passing by reference is unnecessary and could be misleading. Consider passingdby value to maintain consistency and clarity.Suggested change:
-int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t &d) +int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t d)
Line range hint
100-115: Potential buffer overrun indebug_printfIn
debug_printf,vsnprintfis called withsizeof(buffer) - 1, which may prevent proper null-termination if the formatted output exactly fills the buffer. This could lead to undefined behavior when using the buffer contents.Consider using
sizeof(buffer)to ensure the buffer is correctly null-terminated. Also, verify thatlendoes not exceedsizeof(buffer) - 1before writing toDebuggerPort_Write.Suggested change:
- int len = vsnprintf(buffer, sizeof(buffer) - 1, format, arg_ptr); + int len = vsnprintf(buffer, sizeof(buffer), format, arg_ptr);
Line range hint
145-160: Potential out-of-bounds write inhal_strncpy_sIn
hal_strncpy_s, whensizeInBytesis less thancount + 1, the function writes a null terminator tostrDst[0]. IfsizeInBytesis zero, this could result in an out-of-bounds write. EnsurestrDsthas enough space before writing to it.Suggested change:
if (sizeInBytes < count + 1) { _ASSERTE(FALSE); + if (sizeInBytes > 0 && strDst != NULL) + { + strDst[0] = '\0'; + } return 1; }
Line range hint
165-170: Missing NULL pointer check inhal_strlen_sThe function
hal_strlen_sdoes not check ifstrisNULLbefore dereferencing it. Passing aNULLpointer will lead to undefined behavior. Add aNULLcheck to enhance robustness.Suggested change:
+ if (str == NULL) + { + _ASSERTE(FALSE); + return 0; + }
Line range hint
185-200: Missing NULL pointer checks inhal_stricmpThe
hal_stricmpfunction does not verify ifdstorsrcareNULLbefore usage. Dereferencing aNULLpointer results in undefined behavior. It's recommended to addNULLchecks for bothdstandsrc.Suggested change:
+ if (dst == NULL || src == NULL) + { + _ASSERTE(FALSE); + return 0; + }targets/TI_SimpleLink/_nanoCLR/nanoCRT.cpp (5)
Line range hint
46-87: Avoid using C++ references in function parametersThe function
hal_snprintf_doubleuses a C++ reference parameterint64_t &d, which may cause compatibility issues if the code is compiled as C. Since this file is intended for use in embedded systems where C is often preferred, consider passing the parameter by value or using a pointer instead.Apply this diff to modify the function signature:
-int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t &d) +int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t d)Adjust the function implementation and any calls to this function accordingly.
Line range hint
127-147: Potential buffer overflow inhal_strncpy_sIn the
hal_strncpy_sfunction, assigningstrDst[count] = 0;can cause a buffer overflow ifcountis equal to or exceedssizeInBytes, as it writes beyond the allocated buffer. Additionally,strncpydoes not guarantee null-termination if the source string is longer thancount.Apply this diff to ensure safe copying and proper null-termination:
int hal_strncpy_s(char *strDst, size_t sizeInBytes, const char *strSrc, size_t count) { NATIVE_PROFILE_PAL_CRT(); -#undef strncpy if (strDst == NULL || strSrc == NULL || sizeInBytes == 0) { _ASSERTE(FALSE); return 1; } - if (sizeInBytes < count + 1) + if (sizeInBytes <= count) { _ASSERTE(FALSE); strDst[0] = 0; return 1; } - strDst[count] = 0; strncpy(strDst, strSrc, count); + strDst[count] = 0; // Ensure null-termination return 0; -#define strncpy DoNotUse_*strncpy[] }Alternatively, consider using a safer string copy function that guarantees null-termination, such as
strlcpyif available.
Line range hint
108-125: Unnecessary macro manipulation inhal_strcpy_sThe use of
#undef strcpyand redefining it can lead to confusion and maintenance challenges. It might suppress the use ofstrcpyelsewhere unintentionally. Consider removing these macro manipulations and ensure thatstrcpyis used safely within this function.
Line range hint
152-175: Simplify error handling inhal_strncmp_sThe function uses
_ASSERTE(FALSE);when encountering null pointers. Ensure that_ASSERTEis defined in your environment. If not, consider using standardassertor handle the error appropriately. Also, the macro manipulation withstrncmpmay be unnecessary and could be removed for clarity.
Line range hint
178-192: Refactorhal_stricmpfor readabilityThe
hal_stricmpfunction manually adjusts character cases for comparison. You can simplify this by using thetolowerfunction from<ctype.h>, which enhances readability and maintainability.Apply this diff to refactor the function:
+#include <ctype.h> int hal_stricmp(const char *dst, const char *src) { - int f = 0, l = 0; + int f, l; do { - if (((f = (unsigned char)(*(dst++))) >= 'A') && (f <= 'Z')) - { - f -= 'A' - 'a'; - } - if (((l = (unsigned char)(*(src++))) >= 'A') && (l <= 'Z')) - { - l -= 'A' - 'a'; - } + f = tolower((unsigned char)*dst++); + l = tolower((unsigned char)*src++); } while (f && (f == l)); return (f - l); }Ensure that you include
<ctype.h>at the top of the file.targets/ESP32/_nanoCLR/nanoCRT.cpp (4)
Line range hint
29-54: Refactorhal_snprintf_floatto eliminate code duplicationThe
hal_snprintf_floatfunction contains similar logic in both the negative and positive number handling blocks. Consider refactoring to reduce code duplication, which will enhance maintainability and readability.You can combine the common code and handle the sign separately:
int hal_snprintf_float(char *buffer, size_t len, const char *format, int32_t f) { NATIVE_PROFILE_PAL_CRT(); uint32_t i; uint32_t dec; + char sign = '\0'; if (f < 0) { // negative number i = (uint32_t)-f; + sign = '-'; } else { // positive number i = (uint32_t)f; } dec = i & ((1 << HAL_FLOAT_SHIFT) - 1); i = i >> HAL_FLOAT_SHIFT; if (dec != 0) dec = (dec * (uint32_t)HAL_FLOAT_PRECISION + (1 << (HAL_FLOAT_SHIFT - 1))) >> HAL_FLOAT_SHIFT; - if (f < 0) - return hal_snprintf(buffer, len, "-%d.%03u", i, (uint32_t)dec); - else - return hal_snprintf(buffer, len, "%d.%03u", i, (uint32_t)dec); + if (sign) + return hal_snprintf(buffer, len, "%c%d.%03u", sign, i, dec); + else + return hal_snprintf(buffer, len, "%d.%03u", i, dec); }
Line range hint
55-89: Refactorhal_snprintf_doubleto eliminate code duplicationSimilar to
hal_snprintf_float, thehal_snprintf_doublefunction has redundant code for handling negative and positive numbers. Refactoring will improve code clarity and reduce maintenance efforts.Refactored version:
int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t &d) { NATIVE_PROFILE_PAL_CRT(); uint64_t i; uint32_t dec; // 32-bit is enough for decimal part + char sign = '\0'; if (d < 0) { // negative number i = (uint64_t)-d; + sign = '-'; } else { // positive number i = (uint64_t)d; } i += ((1 << (HAL_DOUBLE_SHIFT - 1)) / HAL_DOUBLE_PRECISION); // rounding increment before split dec = i & ((1 << HAL_DOUBLE_SHIFT) - 1); i = i >> HAL_DOUBLE_SHIFT; if (dec != 0) dec = (dec * HAL_DOUBLE_PRECISION + ((1 << (HAL_DOUBLE_SHIFT - 1)) % HAL_DOUBLE_PRECISION)) >> HAL_DOUBLE_SHIFT; - if (d < 0) - return hal_snprintf(buffer, len, "-%lld.%04u", (int64_t)i, (uint32_t)dec); - else - return hal_snprintf(buffer, len, "%lld.%04u", (int64_t)i, (uint32_t)dec); + if (sign) + return hal_snprintf(buffer, len, "%c%lld.%04u", sign, (int64_t)i, dec); + else + return hal_snprintf(buffer, len, "%lld.%04u", (int64_t)i, dec); }
Line range hint
55-55: Consider changing the parameter from reference to valueThe function
hal_snprintf_doubleacceptsint64_t &das a parameter. Since the function does not modifyd, and to maintain consistency with the rest of the codebase, consider passingdby value instead of by reference.-int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t &d) +int hal_snprintf_double(char *buffer, size_t len, const char *format, int64_t d)
Line range hint
29-89: Ensure unit tests cover the new formatting functionsTo verify the correctness of
hal_snprintf_floatandhal_snprintf_double, please ensure these functions are thoroughly tested. Adequate unit tests will help catch edge cases and potential bugs related to floating-point formatting.Do you want me to assist in writing unit tests for these functions or open a GitHub issue to track this task?
targets/AzureRTOS/_common/nanoCLR/nanoCRT.cpp (1)
Line range hint
94-98: Handle potential negative return value fromvsnprintfThe
vsnprintffunction may return a negative value if an encoding error occurs. Using a negativeleninDebuggerPort_Writecan lead to undefined behavior.Consider adding a check for negative
lenbefore proceeding:int len = vsnprintf(buffer, sizeof(buffer) - 1, format, arg_ptr); +if (len < 0) { + va_end(arg_ptr); + return; +} DebuggerPort_Write(HalSystemConfig.stdio, buffer, len, 0); // skip null terminatorThis ensures that you do not attempt to write an invalid number of bytes, and the function exits gracefully on error.
targets/ESP32/_Network/NF_ESP32_OpenThread.cpp (11)
Line range hint
293-293: Usedelete[]when deallocating array allocated withnew[]In the
OpenThreadCliInputfunction, you are allocating memory usingnew[], but deallocating it withdelete. This leads to undefined behavior. When allocating memory withnew[], you must deallocate it withdelete[].Apply this diff to fix the deallocation:
-delete cliLine; +delete[] cliLine;
Line range hint
215-215: Correct the buffer size calculation invsnprintfto prevent buffer overflowIn the
cliOutputCallbackfunction, the buffer size passed tovsnprintfmay lead to a buffer overflow. The size parameter should reflect the remaining space in the buffer starting from the current index.Apply this diff to correct the buffer size calculation:
-vsnprintf(&cliLineBuffer[cliLineBufferCount], cliBufferSize - length, aFormat, aArguments); +vsnprintf(&cliLineBuffer[cliLineBufferCount], cliBufferSize - cliLineBufferCount, aFormat, aArguments);Explanation:
cliBufferSize - cliLineBufferCountcorrectly computes the remaining space incliLineBuffer.
Line range hint
204-204: Avoid usingvsnprintfwithnullptrand zero sizeUsing
vsnprintfwith anullptrdestination and zero size is non-standard and may lead to undefined behavior depending on the implementation ofnanoprintf. It's safer to use an alternative method to calculate the required buffer length.Consider replacing the length calculation with a safer alternative. For example:
-int length = vsnprintf(nullptr, 0, aFormat, aArguments); +int length = npf_vsnprintf(nullptr, 0, aFormat, aArguments);Or, if
nanoprintfdoes not support this usage, consider a workaround:char tempBuffer[1]; int length = vsnprintf(tempBuffer, 0, aFormat, aArguments);Ensure that this usage is supported by
nanoprintf.
Line range hint
138-150: Add bounds checking to prevent buffer overflow inhexTobinfunctionThe
hexTobinfunction does not check the size of thetargetbuffer, which may lead to a buffer overflow if thesrcstring is longer than thetargetbuffer can hold.Modify the function to accept the size of the
targetbuffer and add bounds checking. Here's a suggested refactor:-int hexTobin(const char *src, uint8_t *target) +int hexTobin(const char *src, uint8_t *target, size_t targetSize) { int len = 0; while (*src && src[1]) { + if (len >= targetSize) { + // Prevent buffer overflow + break; + } *(target++) = charTobyte(*src) * 16 + charTobyte(src[1]); src += 2; len++; } return len; }Remember to update all calls to
hexTobinto pass thetargetSizeparameter.
Line range hint
278-284: Potential race condition withcliHandleResponseAccess to
cliHandleResponseis not synchronized, which could lead to race conditions ifOpenThreadCliInputandcliOutputCallbackare called from different threads.Consider adding synchronization mechanisms like mutexes to protect shared variables:
std::mutex cliMutex; void OpenThreadCliInput(const char *inputLine, bool response) { std::lock_guard<std::mutex> lock(cliMutex); // ... rest of the code ... } int cliOutputCallback(void *aContext, const char *aFormat, va_list aArguments) { std::lock_guard<std::mutex> lock(cliMutex); // ... rest of the code ... }Ensure that all accesses to shared variables like
cliHandleResponse,cliOutputsList, andcliLineBufferare protected.
Line range hint
87-116: Improve readability ofsetDeviceTypefunction with consistent indentationThe indentation within the
setDeviceTypefunction is inconsistent, which may reduce code readability.Consider adjusting the indentation to improve readability:
static void setDeviceType(ThreadDeviceType devType, uint32_t pollPeriod) { otInstance *instance = esp_openthread_get_instance(); otLinkModeConfig linkMode = {linkMode.mRxOnWhenIdle = true}; switch (devType) { - case ThreadDeviceType_Router: + case ThreadDeviceType_Router: linkMode.mRxOnWhenIdle = true; linkMode.mDeviceType = true; linkMode.mNetworkData = true; break; - case ThreadDeviceType_EndDevice: + case ThreadDeviceType_EndDevice: linkMode.mRxOnWhenIdle = true; // Not FTD (end device) linkMode.mDeviceType = false; linkMode.mNetworkData = false; break; - case ThreadDeviceType_SleepyEndDevice: + case ThreadDeviceType_SleepyEndDevice: // Not receiving all the time linkMode.mRxOnWhenIdle = false; linkMode.mDeviceType = false; linkMode.mNetworkData = false; // Set poll period for polling if (otLinkSetPollPeriod(instance, pollPeriod) != OT_ERROR_NONE) { ESP_LOGE(TAG, "Failed to set OpenThread poll period."); } powerSaveInit(); break; } if (otThreadSetLinkMode(instance, linkMode) != OT_ERROR_NONE) { ESP_LOGE(TAG, "Failed to set OpenThread linkmode."); } }
Line range hint
215-215: Check return value ofvsnprintffor errorsThe
vsnprintffunction may return a negative value if an encoding error occurs. Not checking its return value might lead to incorrect behavior.Consider checking the return value of
vsnprintf:-int length = vsnprintf(&cliLineBuffer[cliLineBufferCount], cliBufferSize - cliLineBufferCount, aFormat, aArguments); +int result = vsnprintf(&cliLineBuffer[cliLineBufferCount], cliBufferSize - cliLineBufferCount, aFormat, aArguments); +if (result < 0) { + // Handle error + return result; +} +// Update count +cliLineBufferCount += result;This ensures that encoding errors are properly handled.
Line range hint
278-293: Avoid memory leak inOpenThreadCliInputif exceptions occurIf an exception is thrown after allocating
cliLine, the memory may not be deallocated.Consider using smart pointers for automatic memory management:
-char *cliLine = new char[length + 1]; +std::unique_ptr<char[]> cliLine(new char[length + 1]); hal_strcpy_s(cliLine.get(), length + 1, inputLine); otCliInputLine(cliLine.get()); -delete[] cliLine; +// No need to delete manually; `unique_ptr` handles it.This ensures that memory is freed even if an exception occurs.
Line range hint
343-369: SimplifyinitOpenThreadfunction by avoiding magic numbersThe function uses hardcoded numbers for error checking and configuration. Using named constants or enumerations improves readability.
Define error codes and configuration values as constants or enums:
const int EVENT_FD_MAX = 3; const int DEFAULT_STACK_SIZE = 10000;Update the code to use these constants.
Line range hint
87-116: Validate device type input insetDeviceTypeThere is no default case to handle unexpected
devTypevalues, which might lead to uninitializedlinkModeconfigurations.Consider adding a default case to handle invalid
devTypeinputs:switch (devType) { // Existing cases... break; + default: + ESP_LOGE(TAG, "Invalid ThreadDeviceType value."); + return; }
Add
nullptrchecks forpskcandurlbefore callingotJoinerStart.Ensure that both
pskcandurlare validated to be non-null before invokingotJoinerStartto prevent potential crashes or undefined behavior.
- File:
targets/ESP32/_Network/NF_ESP32_OpenThread.cpp- Lines: 393-396
🔗 Analysis chain
Line range hint
393-396: Handle possiblenullptrforpskcandurlinJoinerStartThe
otJoinerStartfunction may not handlenullptrvalues forpskcandurlproperly, leading to unexpected behavior or crashes.Please verify that
pskcandurlare notnullptrbefore callingotJoinerStart, or ensure thatotJoinerStartsafely handlesnullptrvalues.Run the following script to check for
nullptrhandling inotJoinerStart:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `otJoinerStart` handles `nullptr` for `pskc` and `url`. # Using `ast-grep` to find the `otJoinerStart` function definition ast-grep --lang c \ --pattern $'otError otJoinerStart($_, $_, $_, $_, $_, $_, $_, $_, $_)' \ --json # Examine the function implementation to ensure it handles `nullptr` appropriately.Length of output: 444
Script:
#!/bin/bash # Description: Find all calls to `otJoinerStart` and display surrounding code to check handling of `pskc` and `url`. rg 'otJoinerStart' -C 2Length of output: 435
src/CLR/CorLib/corlib_native_System_Number.cpp (1)
Line range hint
89-92: Remove redundant casting inDoPrintfOnDataTypeforDATATYPE_I8The double
(int)(int)cast before thesnprintffunction appears redundant and may be a typographical error. It should be corrected to a single(int)cast for consistency and correctness.Apply this diff to fix the redundant casting:
case DATATYPE_I8: - ret = (int)(int) + ret = (int) snprintf(buffer, FORMAT_RESULT_BUFFER_SIZE, formatStr, (CLR_INT64_TEMP_CAST)value->NumericByRef().s8); break;src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
571-574: Typographical correction in comment for clarityIn the comment at line 572:
Integer, character and stringConsider adding a comma after "character" for proper punctuation:
-Integer, character and string +Integer, character, and string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- CMake/Modules/FindNF_CoreCLR.cmake (3 hunks)
- src/CLR/CorLib/CorLib.h (1 hunks)
- src/CLR/CorLib/CorLib.vcxproj (5 hunks)
- src/CLR/CorLib/corlib_native_System_Number.cpp (1 hunks)
- src/CLR/Core/Core.vcxproj (2 hunks)
- src/CLR/Core/Core.vcxproj.filters (1 hunks)
- src/CLR/Diagnostics/Diagnostics.h (1 hunks)
- src/CLR/Diagnostics/Diagnostics.vcxproj (4 hunks)
- src/CLR/Diagnostics/Info_Safeprintf.cpp (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.h (1 hunks)
- targets/AzureRTOS/_common/nanoCLR/nanoCRT.cpp (1 hunks)
- targets/ChibiOS/_nanoCLR/nanoCRT.cpp (1 hunks)
- targets/ESP32/_Network/NF_ESP32_OpenThread.cpp (1 hunks)
- targets/ESP32/_nanoCLR/nanoCRT.cpp (1 hunks)
- targets/FreeRTOS/_common/nanoCLR/nanoCRT.cpp (1 hunks)
- targets/TI_SimpleLink/_nanoCLR/nanoCRT.cpp (1 hunks)
🔇 Additional comments (31)
src/CLR/Diagnostics/Diagnostics.h (2)
Line range hint
1-17: Overall changes align with PR objectives, but some clarifications needed.The changes in this file partially align with the PR objectives by including the
nanoprintf.hheader, which is likely part of the es-printf library implementation mentioned in the PR summary. However, there are a couple of points that need clarification:
The PR objectives don't mention the
VIRTUAL_DEVICEconditional compilation. Could you please explain why this distinction is necessary and how it relates to the es-printf implementation?The PR summary mentions reverting a previous commit, but this file doesn't show any reverted changes. Are the reverted changes in other files not included in this review?
The commented-out includes weren't mentioned in the PR objectives. It would be helpful to understand if these are related to the changes being made or if they should be addressed separately.
These clarifications will help ensure that the changes fully meet the PR objectives and maintain the overall code quality.
To verify the changes related to the es-printf library implementation, please run the following script:
#!/bin/bash # Description: Check for es-printf related changes and usage # Search for es-printf related files echo "Searching for es-printf related files:" fd -e c -e h -e cpp "es.*printf" # Search for nanoprintf usage echo "Searching for nanoprintf usage:" rg --type cpp "nanoprintf" -C 2 # Check if there are any other printf-related changes echo "Checking for other printf-related changes:" git diff --name-only | xargs rg --type cpp "printf" -C 2
14-16: LGTM. Please verify VIRTUAL_DEVICE usage and add documentation.The conditional inclusion of
nanoprintf.haligns with the PR objective of reinstating the printf implementation. This change looks good, but there are a couple of points to consider:
Please verify that the
VIRTUAL_DEVICEmacro is consistently used across the project for distinguishing between virtual and non-virtual device builds.It would be helpful to add a brief comment explaining why
nanoprintf.his excluded for virtual devices. This will provide context for future maintainers.To ensure consistent usage of the
VIRTUAL_DEVICEmacro, please run the following script:src/CLR/CorLib/CorLib.h (1)
17-19: LGTM! Consider regrouping includes.The addition of the
nanoprintf.hinclude aligns well with the PR objective of reinstating the printf implementation. The conditional compilation ensures it's only included when not building for a virtual device, which is a good practice for managing different build configurations.Consider moving this include statement to group it with other similar includes, perhaps after the
#include "nanoCRT.h"line. This would improve code organization and readability.Let's verify the impact of this change on the rest of the codebase:
This script will help us understand how
nanoprintf.his used across the project and identify any potential conflicts or inconsistencies.✅ Verification successful
Change Verified
The inclusion of
nanoprintf.hhas been confirmed across the codebase without introducing any conflicts or inconsistencies. The refactor suggestion to regroup the include statement with similar includes remains valid for improved code organization and readability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential conflicts or inconsistencies with nanoprintf usage # Test 1: Check if nanoprintf.h exists in the project echo "Checking if nanoprintf.h exists:" fd nanoprintf.h # Test 2: Look for other inclusions of nanoprintf.h echo "Checking for other inclusions of nanoprintf.h:" rg '#include.*nanoprintf\.h' # Test 3: Check for potential conflicts with printf-like functions echo "Checking for potential conflicts with printf-like functions:" rg '\b(sprintf|fprintf|printf|snprintf)\b' --type cpp # Test 4: Check for usage of VIRTUAL_DEVICE macro echo "Checking for usage of VIRTUAL_DEVICE macro:" rg 'VIRTUAL_DEVICE' --type cppLength of output: 20323
src/CLR/Diagnostics/Info_Safeprintf.cpp (3)
8-8: LGTM: Addition of nanoprintf.hThe inclusion of
nanoprintf.his consistent with the PR objectives of reintroducing the printf implementation from the es-printf library.
14-15: Improved error handling for vsnprintfThe changes to the return type and condition for
vsnprintfare correct and beneficial:
- Using
intinstead ofsize_tforcharsmatches the return type ofvsnprintf.- The new condition
chars >= 0properly checks for encoding errors (negative return) and successful formatting (non-negative return).These modifications enhance the robustness of the error handling in this function.
17-18: Simplified and improved buffer overflow handlingThe changes to the buffer overflow handling are appropriate:
- The condition
fRes == falsecorrectly identifies both encoding errors and buffer overflows.- Setting
chars = (int)iBufferin case of failure ensures that the entire buffer is marked as used, preventing potential buffer overruns.This simplification improves the safety and clarity of the function.
src/CLR/Core/Core.vcxproj.filters (1)
Line range hint
1-203: Summary: Change aligns with PR objectivesThe addition of
nanoprintf.cto the project structure successfully reintroduces the printf implementation as intended. This change is minimal and focused, which is a good practice for pull requests. The file is correctly placed within the "Source Files" filter, ensuring it will be compiled as part of the project.To ensure that this change doesn't conflict with any existing printf implementations, please run the following verification script:
This script will help verify that we're not introducing conflicting implementations and that nanoprintf.c is the only new printf-related file being added.
✅ Verification successful
To ensure that there are no conflicting
printfimplementations introduced by addingnanoprintf.c, please run the following verification script:
Verification Successful: No Conflicting
printfImplementations FoundThe addition of
nanoprintf.csuccessfully integrates the newprintfimplementation without introducing conflicts. The existing macro definitions appropriately redirectprintfcalls tonanoprintf, ensuring consistency across the project.
nanoprintf.candnanoprintf.hare correctly added to the "Source Files" filter.- No other
printfimplementations were detected that could conflict withnanoprintf.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing printf implementations that might conflict with nanoprintf # Search for other printf implementations echo "Searching for other printf implementations:" rg -i 'printf.*implementation' --type c --type cpp --type h # Check if nanoprintf.c is the only new printf-related file echo "Checking for new printf-related files:" git diff --name-only | grep -i 'printf'Length of output: 285
Script:
#!/bin/bash # Description: Comprehensive search for printf implementations and usages to ensure no conflicts with nanoprintf echo "Searching for printf function definitions or overrides:" rg -i '^\s*(?:int|void)\s+printf\s*\(' --type c --type cpp --type h echo "Searching for printf macro definitions:" rg -i '#\s*define\s+PRINTF' --type c --type cpp --type h echo "Listing all printf-related files in the repository:" fd -i 'printf' --type fLength of output: 1352
src/CLR/Diagnostics/Diagnostics.vcxproj (3)
108-108: LGTM: Consistent changes across all configurations.The addition of the
..\Helpers\nanoprintfinclude directory is consistent across all build configurations (Debug|Win32, Debug|x64, Release|Win32, Release|x64). This is a good practice as it ensures the same behavior across different build types.Also applies to: 121-121, 136-136, 153-153
108-108: Check the order of include directories.The new include directory
..\Helpers\nanoprintfhas been added as the first entry in theAdditionalIncludeDirectoriesproperty. While this ensures that the new printf implementation takes precedence, please verify that this order doesn't cause any unintended consequences or conflicts with other headers.If the script finds any matches, review those files to ensure there are no conflicts between the nanoprintf implementation and standard library headers.
Also applies to: 121-121, 136-136, 153-153
✅ Verification successful
Include directory order verified successfully; no conflicts detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential header conflicts # Test: Search for files that include both nanoprintf and potentially conflicting headers rg --type cpp -e '#include.*nanoprintf' -e '#include.*<stdio.h>' -e '#include.*<printf.h>' src/Length of output: 576
108-108: Verify the necessity of the old include directory.The old include directory
..\..\..\targets\win32\Includeis still present in theAdditionalIncludeDirectoriesproperty. Please verify if this directory is still necessary after the introduction of the nanoprintf library.If the script doesn't find any matches, consider removing the old include directory.
Also applies to: 121-121, 136-136, 153-153
✅ Verification successful
Remove the old include directory
..\..\..\targets\win32\IncludefromAdditionalIncludeDirectories, as no references were found after introducing the nanoprintf library.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any files in the project that still include headers from the old directory # Test: Search for includes from the old directory rg --type cpp -g '!**/nanoprintf/*' '#include.*targets/win32/Include' src/Length of output: 76
Script:
#!/bin/bash # Description: Re-verify if any files still include headers from the old directory # Test: Search for includes from the old directory with corrected pattern rg --type cpp -e '#include\s*["<]..\..\..\targets/win32/Include[^">]*[">]' src/Length of output: 81
Script:
#!/bin/bash # Description: Search for any #include statements referencing the targets/win32/Include directory # Test: Search for includes from the old directory using a flexible pattern rg --type cpp '#include\s*["<].*targets/win32/Include[^">]*[">]' src/Length of output: 71
src/CLR/Core/Core.vcxproj (1)
22-22: LGTM! Verify nanoprintf integration.The addition of
nanoprintf.cto the project is consistent with the PR objective of reinstating the printf implementation. The file is correctly added to theClCompileitem group, ensuring it will be compiled as part of the project.To ensure the nanoprintf library is properly integrated, please run the following verification script:
This script will help verify the presence of the nanoprintf file, its usage in the project, and potential conflicts with existing printf implementations.
✅ Verification successful
Verified: nanoprintf integration is correct.
The
nanoprintf.cfile exists atsrc/CLR/Helpers/nanoprintf/nanoprintf.cand is correctly referenced in theCore.vcxprojfile. The integration aligns with the PR objectives, and there are no conflicts with existingprintfimplementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence and usage of nanoprintf in the project. # Test 1: Check if the nanoprintf.c file exists echo "Checking for nanoprintf.c file..." fd -t f "nanoprintf.c" src/Helpers/nanoprintf # Test 2: Search for nanoprintf function usage in the project echo "Searching for nanoprintf usage..." rg --type cpp "nanoprintf" src # Test 3: Check for any potential conflicts with existing printf implementations echo "Checking for potential conflicts..." rg --type cpp "printf" src | grep -v "nanoprintf"Length of output: 12646
Script:
#!/bin/bash # Description: Locate the nanoprintf.c file within the project. echo "Searching for nanoprintf.c file in the entire project..." fd -t f "nanoprintf.c" srcLength of output: 189
src/CLR/CorLib/CorLib.vcxproj (1)
155-155: LGTM: Consistent addition of nanoprintf include directory.The changes consistently add the
..\..\CLR\Helpers\nanoprintfdirectory to theAdditionalIncludeDirectoriesproperty across all configurations (Debug/Release) and platforms (Win32/x64). This is a good practice for maintaining build consistency.To ensure the nanoprintf directory exists and contains the expected files, please run the following script:
This script will verify the existence of the nanoprintf directory and check for essential files. Please adjust the list of essential files as needed based on the expected contents of the nanoprintf library.
Also applies to: 168-168, 183-183, 200-200
✅ Verification successful
Verified: nanoprintf directory and essential files confirmed.
The
..\..\CLR\Helpers\nanoprintfdirectory exists and contains the essential files (nanoprintf.c,nanoprintf.h) across all configurations and platforms, ensuring build consistency and intended functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and contents of the nanoprintf directory # Check if the directory exists if [ -d "src/CLR/Helpers/nanoprintf" ]; then echo "nanoprintf directory exists." # List the contents of the directory ls -l src/CLR/Helpers/nanoprintf else echo "Error: nanoprintf directory not found." exit 1 fi # Check for essential files (adjust the list as needed) essential_files=("nanoprintf.h" "nanoprintf.c") for file in "${essential_files[@]}"; do if [ -f "src/CLR/Helpers/nanoprintf/$file" ]; then echo "Found essential file: $file" else echo "Error: Essential file not found: $file" exit 1 fi done echo "Verification complete."Length of output: 721
CMake/Modules/FindNF_CoreCLR.cmake (4)
271-271: LGTM: nanoprintf directory added to find_file paths.The addition of the nanoprintf directory to the find_file paths is correct and consistent with the earlier changes. This ensures that the build system can locate the nanoprintf.c file when processing the source files list.
25-25: Summary: Clean integration of nanoprintf libraryThe changes in this file successfully integrate the nanoprintf library into the NF_CoreCLR build process. The additions include:
- A new include directory for nanoprintf headers
- The nanoprintf.c source file in the compilation list
- The nanoprintf directory in the find_file paths
These changes are consistent, minimal, and follow the existing structure of the CMake module. They should enable the use of nanoprintf functionality in the NF_CoreCLR library without introducing conflicts or issues.
Also applies to: 146-146, 271-271
25-25: LGTM: New include directory for nanoprintf added correctly.The addition of the nanoprintf include directory is consistent with the existing structure and will allow the compiler to find the necessary header files.
To ensure the directory structure is correct, please run the following command:
#!/bin/bash # Verify the existence of the nanoprintf directory if [ -d "${CMAKE_SOURCE_DIR}/src/CLR/Helpers/nanoprintf" ]; then echo "nanoprintf directory exists." else echo "nanoprintf directory not found. Please check the path." fi
146-146: LGTM: nanoprintf.c added to the source files list.The addition of nanoprintf.c to the NF_CoreCLR_SRCS list is correct and ensures it will be compiled as part of the NF_CoreCLR library.
To confirm the existence of the nanoprintf.c file, please run the following command:
✅ Verification successful
LGTM: nanoprintf.c added to the source files list.
The addition of
nanoprintf.cto theNF_CoreCLR_SRCSlist is correct and ensures it will be compiled as part of the NF_CoreCLR library.
- Verified the existence of
nanoprintf.catsrc/CLR/Helpers/nanoprintf/nanoprintf.c.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of nanoprintf.c if [ -f "${CMAKE_SOURCE_DIR}/src/CLR/Helpers/nanoprintf/nanoprintf.c" ]; then echo "nanoprintf.c file exists." else echo "nanoprintf.c file not found. Please check the path." fiLength of output: 159
Script:
#!/bin/bash # Search for nanoprintf.c in the repository rg --files | grep 'nanoprintf\.c$'Length of output: 76
targets/ChibiOS/_nanoCLR/nanoCRT.cpp (2)
8-8: Including<nanoprintf.h>to utilize nanoprintf libraryThe inclusion of
<nanoprintf.h>enables the use of the nanoprintf library, providing efficient and lightweight printf functionalities suitable for embedded systems.
Line range hint
168-187: Document limitations of ASCII-only comparison inhal_stricmpThe
hal_stricmpfunction compares strings in a case-insensitive manner but is limited to ASCII characters and does not consider locale-specific rules or extended character sets.Ensure that this function is suitable for your application's requirements and add documentation to highlight its ASCII-only limitation.
Would you like assistance in implementing a locale-aware, Unicode-compliant case-insensitive comparison function?
targets/FreeRTOS/_common/nanoCLR/nanoCRT.cpp (1)
8-8: Including<nanoprintf.h>headerThe inclusion of
<nanoprintf.h>integrates thenanoprintflibrary, which enhances formatting capabilities. This is appropriate and necessary for the added functionality.targets/TI_SimpleLink/_nanoCLR/nanoCRT.cpp (2)
8-8: Verify the inclusion of<nanoprintf.h>Ensure that including
<nanoprintf.h>is necessary and that it does not conflict with existing headers or definitions in the project. Confirm that this header provides the required functionality without introducing any redundancy.
Line range hint
13-45: Avoid magic numbers and ensure constants are definedIn the
hal_snprintf_floatfunction, constants likeHAL_FLOAT_SHIFTandHAL_FLOAT_PRECISIONare used. Verify that these constants are defined appropriately and consider replacing magic numbers with well-defined constants or enumerations for better readability and maintainability.targets/ESP32/_nanoCLR/nanoCRT.cpp (2)
9-9: Including<nanoprintf.h>is appropriate and necessaryThe inclusion of
<nanoprintf.h>enables the use of thenanoprintflibrary for formatted printing functionalities, which aligns with the PR objective of adding back the private implementation ofprintf.
Line range hint
29-89: Verify buffer safety inhal_snprintf_*functionsEnsure that the usage of
hal_snprintfwithinhal_snprintf_floatandhal_snprintf_doubleproperly handles the buffer size (len) to prevent potential buffer overflows.To confirm buffer safety, you can run the following script:
targets/AzureRTOS/_common/nanoCLR/nanoCRT.cpp (5)
9-9: Including<nanoprintf.h>for enhanced formatting capabilitiesThe inclusion of
<nanoprintf.h>integrates the nanoprintf library, which enhances the project's string formatting capabilities.
Line range hint
23-54: Ensure correctness ofhal_snprintf_floatimplementationThe
hal_snprintf_floatfunction correctly handles the formatting of fixed-point numbers by separating integer and decimal parts for both positive and negative values. The use of bit manipulation and rounding appears appropriate.
Line range hint
56-87: Verify the implementation ofhal_snprintf_doubleThe
hal_snprintf_doublefunction mirrors the logic ofhal_snprintf_float, adapting it for double-precision numbers. Ensure that the calculations, especially the bit shifts and rounding mechanisms, handle edge cases correctly.
Line range hint
116-132: Appropriate handling of safe string copy inhal_strcpy_sThe
hal_strcpy_sfunction includes necessary checks for null pointers and buffer sizes, ensuring safe string copying. The use of_ASSERTE(FALSE)helps catch errors during debugging.
Line range hint
135-151: Safe implementation ofhal_strncpy_sThe
hal_strncpy_sfunction correctly handles boundary conditions and ensures that the destination string is null-terminated. The checks for null pointers and buffer sizes enhance robustness.src/CLR/Helpers/nanoprintf/nanoprintf.c (3)
1085-1090: Ensure proper handling of buffer boundaries indoprntfunctionThe added condition checks if
count >= nand breaks out of the loop to prevent writing beyond the buffer size. This change helps in avoiding buffer overflows and ensures that the output stays within the allocated memory.
264-265:⚠️ Potential issueReview handling of
DBL_MINin floating-point normalizationIn the normalization code for handling edge cases:
else if(number == DBL_MIN) { number = -1.7976931348623157; decpt = -309; }Assigning a negative value to
numberwhenDBL_MINis the smallest positive normalized double may not be appropriate and could introduce inaccuracies in formatting.Please verify that the handling of
DBL_MINaligns with the expected behavior for the smallest positive double value. It's important to ensure that the value assigned tonumbercorrectly representsDBL_MINwithout changing its sign.Run the following script to double-check constants and usages of
DBL_MIN:#!/bin/bash # Description: Verify that DBL_MIN is correctly used in the codebase. # Search for all occurrences of DBL_MIN rg --type c --word-regexp 'DBL_MIN' # Check for assignments that may incorrectly alter its sign rg --type c 'number\s*=\s*-\s*DBL_MIN'
96-98: Verify increasedFLOAT_DIGITSvalue for potential buffer overflowsThe macro
FLOAT_DIGITShas been increased to18. Ensure that all related buffer sizes and calculations can accommodate this increased precision without causing buffer overflows or other memory issues, especially in functions that useFLOAT_DIGITSfor buffer allocations.Run the following script to identify buffer allocations and usages dependent on
FLOAT_DIGITS:✅ Verification successful
FLOAT_DIGITS increase does not pose buffer overflow risks
After thorough verification, increasing the
FLOAT_DIGITSvalue to18does not affect any buffer sizes or related calculations in the codebase. No potential buffer overflows were identified related to this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of FLOAT_DIGITS and related buffer sizes. # Find all occurrences of FLOAT_DIGITS in the codebase rg --type c --word-regexp 'FLOAT_DIGITS' # Find buffer declarations that may be affected rg --type c --regex 'char\s+\w+\[\s*(BUFMAX|DIGITS_MAX_[FEG]|\d+)\s*\]' # Search for any calculations involving FLOAT_DIGITS that could impact buffer sizes rg --type c --regex 'BUFMAX\s*[\*\/+-]\s*FLOAT_DIGITS'Length of output: 340
Script:
#!/bin/bash # Description: Search for usages of FLOAT_DIGITS and related buffer sizes. # Find all occurrences of FLOAT_DIGITS in the codebase rg --type c --word-regexp 'FLOAT_DIGITS' # Find buffer declarations that may be affected rg --type c --regex 'char\s+\w+\[\s*(BUFMAX|DIGITS_MAX_[FEG]|\d+)\s*\]' # Search for any calculations involving FLOAT_DIGITS that could impact buffer sizes rg --type c --regex 'BUFMAX\s*[\*/+\-]\s*FLOAT_DIGITS'Length of output: 1387
Script:
#!/bin/bash # Description: Search for usages of FLOAT_DIGITS and related buffer sizes. # Find all occurrences of FLOAT_DIGITS in the codebase rg --type c --word-regexp 'FLOAT_DIGITS' # Find buffer declarations that may be affected rg --type c --regexp 'char\s+\w+\[\s*(BUFMAX|DIGITS_MAX_[FEG]|\d+)\s*\]' # Search for any calculations involving FLOAT_DIGITS that could impact buffer sizes rg --type c --regexp 'BUFMAX\s*[\*/+\-]\s*FLOAT_DIGITS'Length of output: 3283
Script:
#!/bin/bash # Description: Search for buffer declarations and buffer-related function usages involving FLOAT_DIGITS. # Find buffer declarations where buffer size uses FLOAT_DIGITS rg --type c --regexp 'char\s+\w+\[\s*.*FLOAT_DIGITS.*\]' # Find buffer-related function calls that utilize FLOAT_DIGITS for formatting rg --type c --regexp 'snprintf\s*\(.*FLOAT_DIGITS.*\)' rg --type c --regexp 'sprintf\s*\(.*FLOAT_DIGITS.*\)' # Find any other buffer size calculations involving FLOAT_DIGITS rg --type c --regexp '(BUFMAX|BUFFER_SIZE|SIZE)\s*[\*/+\-]\s*FLOAT_DIGITS'Length of output: 248
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/CLR/Helpers/nanoprintf/nanoprintf.h (2)
72-116: Good configuration options with room for documentation improvementThe memory access and output configuration sections provide excellent flexibility for different platforms. The commented examples for AVR microcontrollers (lines 98-104) offer valuable guidance for platform-specific implementations.
The custom
putchar_function (line 114) allows for easy output redirection, which is great for embedded systems. However, it would be beneficial to add a brief comment explaining the expected behavior of this function for future maintainers.Consider adding a brief comment above the
putchar_function declaration to explain its purpose and expected implementation.
310-313: Function redefinitions for custom printf implementationThe redefinition of standard printf functions to use custom implementations (lines 310-313) allows for seamless integration of the custom printf functionality. This approach is consistent with the project's requirements, as confirmed in the retrieved learnings.
However, to enhance safety and flexibility, consider wrapping these redefinitions in a conditional compilation block. This would allow users to opt-out of the redefinitions if needed.
Consider wrapping the redefinitions in a conditional compilation block:
+#ifndef NANOPRINTF_NO_REDEFINITION #define printf printf_ #define sprintf sprintf_ #define snprintf snprintf_ #define vsnprintf vsnprintf_ +#endif // NANOPRINTF_NO_REDEFINITIONThis change allows users to disable the redefinitions by defining
NANOPRINTF_NO_REDEFINITIONif they need to use the standard library functions in certain parts of their code.src/CLR/Helpers/nanoprintf/nanoprintf.c (4)
152-171: LGTM! Consider a small optimization for large numbers.The
count_integer_digitsfunction is well-implemented and correctly handles negative numbers, zero, and positive numbers. It efficiently counts the digits in the integer part of a double.For very large numbers, consider using logarithms for a more efficient calculation:
flt_width_t count_integer_digits(double number) { // Handle negative numbers if (number < 0) { number = -number; } // Handle special case for zero if (number < 1.0) { return 1; } + // For large numbers, use logarithm for efficiency + if (number >= 1e6) { + return (flt_width_t)(floor(log10(number))) + 1; + } flt_width_t count = 0; while (number >= 1.0) { number /= 10.0; count++; } return count; }This optimization would be more efficient for very large numbers, while still using the current method for smaller numbers.
300-311: LGTM! Consider adding a comment for clarity.The changes to
format_floatfunction correctly handle the case when the number is an integer. This is a good optimization to avoid unnecessary decimal places for integer values.Consider adding a brief comment explaining the purpose of this check:
// [NF_CHANGE] +// Special handling for integer values to avoid unnecessary decimal places // check if number is integer if (number == (int)number) { // number is an integer // set number of digits required to have 0 decimal places ndigits = count_integer_digits(number); } // [END_NF_CHANGE]This comment would help future maintainers understand the purpose of this optimization quickly.
1261-1278: LGTM! Consider adding error handling for edge cases.The
snprintf_function is well-implemented, correctly handling the buffer size limit and ensuring null-termination. It properly usesdoprntfor formatting and returns the correct count.Consider adding error handling for the case when
nis 0:printf_t snprintf_(char *buf, size_t n,const char *fmt, ... ) { va_list ap; int count; + // Handle edge case where buffer size is 0 + if (n == 0) { + return 0; + } va_start(ap, fmt); count = doprnt(&buf, putbuf, n - 1, fmt, ap); va_end(ap); // Append null terminator if there's space. - if (n > 0) - { - *buf = '\0'; - } + *buf = '\0'; return count; }This change ensures that the function behaves correctly even when given a buffer of size 0, and simplifies the null-termination logic.
1288-1314: LGTM! Consider simplifying null-termination logic.The
vsnprintf_function is well-implemented, correctly usingva_copy,doprnt, and handling buffer size limits. It properly ensures null-termination and returns the correct count.Consider simplifying the null-termination logic and adding a check for buffer size 0:
printf_t vsnprintf_(char *buffer, size_t bufsz, char const *format, va_list vlist) { size_t count; va_list vlistCopy; + // Handle edge case where buffer size is 0 + if (bufsz == 0) { + return 0; + } // Create a copy of the variable argument list va_copy(vlistCopy, vlist); // Perform the actual formatting operation - count = doprnt(&buffer, putbuf, bufsz, format, vlistCopy); + count = doprnt(&buffer, putbuf, bufsz - 1, format, vlistCopy); // append null terminator - if(count < bufsz) - { - buffer[count] = '\0'; - } - else - { - buffer[bufsz - 1] = '\0'; - } + buffer[count < bufsz - 1 ? count : bufsz - 1] = '\0'; // Clean up the copied variable argument list va_end(vlistCopy); return count; }These changes simplify the null-termination logic, ensure that we always leave space for the null terminator when calling
doprnt, and handle the edge case of a 0-size buffer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (1 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.h (1 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.src/CLR/Helpers/nanoprintf/nanoprintf.h (1)
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.
🔇 Additional comments (5)
src/CLR/Helpers/nanoprintf/nanoprintf.h (3)
1-70: Well-structured header with flexible type definitionsThe file header is well-organized, including proper copyright information and include guards. The custom typedef for
printf_t(lines 51-55) provides excellent flexibility, allowing for different return types based on platform requirements. This approach facilitates easy adaptation to various embedded systems.
117-224: Comprehensive and extensible feature flag systemThe feature configuration section provides an extensive and well-documented set of feature flags. Each flag is clearly defined with a unique bit, allowing for efficient combination of features. This approach offers fine-grained control over the printf implementation, which is crucial for optimizing performance and code size in embedded systems.
The clear comments for each flag (e.g., lines 150-151, 153-154) make it easy for developers to understand and select the required features for their specific use case.
1-317: Comprehensive and flexible printf implementation for embedded systemsThis file provides a well-structured and highly configurable implementation of printf-like functions tailored for embedded systems. The extensive use of feature flags and pre-defined feature sets allows for fine-grained control over functionality and code size, which is crucial for resource-constrained environments.
Key strengths:
- Flexible type definitions for different platforms
- Comprehensive feature flag system
- Pre-defined feature sets for common use cases
- Custom output function support
Minor suggestions for improvement have been made, including:
- Adding documentation for the custom
putchar_function- Reviewing the NANOFRAMEWORK_SET for potential optimization
- Adding a conditional compilation block for function redefinitions
Overall, this implementation is well-suited for the nanoFramework project, providing the necessary flexibility and functionality for various embedded systems.
src/CLR/Helpers/nanoprintf/nanoprintf.c (2)
1281-1285: Clarify the purpose of theputchar_function.The
putchar_function is currently implemented as an empty function that does nothing with its input. It's unclear why this function exists in its current form.Could you provide more context on the intended use of this function? If it's meant to be a placeholder for future implementation, consider adding a TODO comment. If it's intentionally empty for some reason, please add a comment explaining why.
// [NF_CHANGE] void putchar_(char character) { + // TODO: Implement character output or explain why this function is intentionally empty (void)character; } // [END_NF_CHANGE]This will help future maintainers understand the purpose of this function.
1-1316: Overall, good improvements to the printf implementation.The changes in this file enhance the safety and functionality of the printf-like functions. The new
count_integer_digitsfunction, modifications toformat_float, and the addition ofsnprintf_andvsnprintf_functions are well-implemented and improve the overall robustness of the code.Some minor suggestions have been made for optimization and clarity:
- Consider using logarithms for large numbers in
count_integer_digits.- Add explanatory comments for the integer handling in
format_float.- Improve error handling in
snprintf_for edge cases.- Clarify the purpose of the empty
putchar_function.- Simplify null-termination logic in
vsnprintf_.These changes will further improve the code's efficiency, readability, and maintainability.
josesimoes
force-pushed
the
add-back-es-printf
branch
from
8a40e1b to
b3afd91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
1-1: Remove Byte Order Mark (BOM) from the start of the fileThe file begins with a Byte Order Mark (BOM) character before the comment on line 1. This can cause issues with compilers or tools that do not expect a BOM. Please remove the BOM to ensure compatibility across different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.
- Now at caller.
josesimoes
force-pushed
the
add-back-es-printf
branch
from
b3afd91 to
af04c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/CLR/CorLib/corlib_native_System_Number.cpp (2 hunks)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLR/CorLib/corlib_native_System_Number.cpp
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.
🔇 Additional comments (2)
src/CLR/Helpers/nanoprintf/nanoprintf.c (2)
1243-1246: Clarify the purpose of the emptyputchar_functionThe
putchar_function is currently implemented as an empty function that does nothing with the input character. This might be intentional, but it's unusual for a function namedputchar_to not actually output the character.Could you please clarify the purpose of this empty implementation? Is it intended to be a placeholder for future implementation, or is it meant to suppress character output in certain contexts?
If it's a placeholder, consider adding a TODO comment to indicate future implementation plans:
void putchar_(char character) { + // TODO: Implement character output or explain why this function is intentionally empty (void)character; }
1-1278: Summary of nanoprintf.c reviewOverall, this file provides a functional implementation of lightweight printf-like functionality. The code demonstrates attention to detail in handling various formatting options and edge cases. However, there are several areas where improvements could be made:
- Enhanced buffer overflow protection in
snprintf_andvsnprintf_functions.- Clarification needed for the empty
putchar_function.- Refactoring opportunities in the
format_floatfunction to improve readability and maintainability.- Improvements in overall file structure and organization, including consistent naming conventions, better grouping of related functions, and more comprehensive documentation.
Addressing these points would significantly enhance the code's robustness, readability, and maintainability. Despite these suggestions for improvement, the implementation appears to be functional and well-thought-out in terms of handling various formatting scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/CLR/Helpers/nanoprintf/nanoprintf.c (1)
1243-1246: Add a comment explaining the emptyputchar_functionThe
putchar_function is currently empty. While this might be intentional, it's not immediately clear why. Consider adding a comment explaining the purpose of this empty function to improve code readability and prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/CLR/Helpers/nanoprintf/nanoprintf.c (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.src/CLR/Helpers/nanoprintf/nanoprintf.c (2)
Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.c:160-570 Timestamp: 2024-10-03T16:46:22.543Z Learning: The `nanoprintf.c` file contains external code that should not be refactored.Learnt from: josesimoes PR: nanoframework/nf-interpreter#3026 File: src/CLR/Helpers/nanoprintf/nanoprintf.h:289-292 Timestamp: 2024-09-30T10:58:31.183Z Learning: In this project, renaming standard library functions like `printf`, `sprintf`, `snprintf`, and `vsnprintf` to custom implementations is intentional and acceptable.
🔇 Additional comments (2)
src/CLR/Helpers/nanoprintf/nanoprintf.c (2)
1223-1240: Well-implementedsnprintf_functionThe
snprintf_function is correctly implemented. It properly handles edge cases, ensures null-termination, and returns the correct count. Good job on following the standardsnprintfbehavior.
1250-1272: Well-implementedvsnprintf_functionThe
vsnprintf_function is correctly implemented. It properly handles edge cases, ensures null-termination, and returns the correct count. The use ofva_copyis a good practice. Well done on following the standardvsnprintfbehavior.
josesimoes
force-pushed
the
add-back-es-printf
branch
from
e96c6b1 to
af04c1d
Compare
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
nanoprintffunctionality for enhanced string formatting, including support for various data types and floating-point numbers.snprintf_,vsnprintf_) and improved handling of numeric formatting.Bug Fixes
Documentation