The provided code in general looks well-structured and following good coding practices for a CMake file. Below are a few notes:
Assumptions about External Packages: The code assumes the availability of the 'simdjson' package. If it is not found, then it fetches the content from GIT_REPOSITORY. That's a good thing, but it also assumes that FetchContent is available with the inclusion command 'include(FetchContent)' and that the other packages like GTEST_BOTH_LIBRARIES and wasmedgeCommon are available when linking libraries. It's better to include checks for these assumptions or provide informative error messages if they aren't met.
MSVC Specifics: There is a specific section for handling MSVC (Microsoft Visual Studio Compiler). If the compiler is not MSVC, the code doesn't perform any operations to handle other compilers like GCC or Clang. It's better to add some error handling or exception handling for other compilers, or at least a message indicating that the required operations are only intended for MSVC.
Hardcoded Library Versions: Versions for the external Git repositories (like 'GIT_TAG wasm-dev-0.13.0' and 'GIT_TAG tags/v3.2.1') are hardcoded. Hardcoding versions is a common practice, but it could lead to outdated versions over time and potentially complicate version management.
Ignored Compiler Warnings: The code ignores a range of compiler warnings for MSVC. This can be helpful for suppressing unnecessary or non-problematic warnings but ignoring too many warnings can lead to overlooking real issues within the code.
Avoid Silent Errors: The failure of the function 'wasmedge_copy_spec_testsuite()' is not checked. If the directories or files cannot be copied for some reason (e.g., lack of permissions, disk space, etc.), the script will not fail and will continue executing the rest of the code, which could inadvertently cause a silent error.
Remember that these comments are based purely on standard good practices and may not apply depending on the specifics of the project, the development environment, and the team's coding conventions.
The patch summarises some key changes in the code:
Library Change: It changes the JSON parsing library from RapidJSON to SIMDJSON. This impacts several subsequent code sections, from the GIT repository the code is fetched from, to the library specific flags used in the configuration.
Changes in the Compiler Flags: Certain compiler warning flags specific to RapidJSON have been removed, and several SIMDJSON compiler warning flags have been added. These flags are specific to the Microsoft Visual Studio Compiler (MSVC).
Static Library Configuration: For SIMDJSON library, two options SIMDJSON_JUST_LIBRARY and SIMDJSON_BUILD_STATIC are set to ON via CACHE INTERNAL. This indicates that SIMDJSON should be built as a static library and just SIMDJSON, nothing else.
Linking Library Change: In the target_link_libraries function, the library name has been changed from RapidJSON to simdjson. This aligns with the earlier change in adopted library.
These changes primarily reflect a decision to change the JSON library in use from RapidJSON to SIMDJSON, with a number of necessary adjustments throughout the code to ensure compatibility.
Missing include guard or #pragma once: Including the same header multiple times can lead to redefinitions or multiple inclusions. To protect your headers, use include guard or #pragma once.
Converting between string view and string: Conversions between std::string_view and std::string are made without checking for null termination. This could potentially introduce bugs.
Lack of error handling: There are several places where functions are called but it is assumed that they will always succeed. Adding proper error handling will make the code more robust, such as Doc = parser.load(FName);, Args = Action["args"];, etc.
Unexpected behavior: assumingUnreachable(); function is called without any apparent check. If this function is supposed to represent an error or abnormal condition, there should be a condition that triggers its call. A simple unreachable code is a smell of a deeper problem.
Using strcmp and strstr functions from the C library can produce undefined behavior if the strings are not null-terminated. Consider using the equivalent std::string::compare and std::string::find functions instead.
If the spdlog::error function does not stop the execution of the program, the stringContains function may introduce a silent fail, misleading the developer about the real status of the execution. If it is a critical problem, consider throwing an exception or stopping the execution.
Magic numbers: Avoid using magic constant numbers in your code. Use named constant variables instead for better readability.
Missing comments: Although the code is mostly well-written and clean, more comments could help readers understand the code easier.
Check the use of reinterpret_cast. It can be risky if not properly used, remember to ensure you're interpreting the data correctly.
Be careful when using std::map::operator[] as it will silently create a new entry in the map, if the key does not already exist.
Additionally, it would be recommended to follow a consistent curly braces {} style for conditional and control structures. In this piece of code, sometimes they're on the same line and sometimes on a new line.
This patch appears to modify a software testing file and includes several significant changes.
The primary objective of these changes appears to focus on replacing the usage of rapidjson with simdjson in various places.
The updates include:
Removal of #define flags related to RAPIDJSON_HAS_STDSTRING, RAPIDJSON_SSE42, RAPIDJSON_SSE2, and RAPIDJSON_NEON replacing rapidjson includes with simdjson.
Modification of several helper functions including GetModuleName, Invoke, Get, TrapInvoke, RunCommand, etc with simdjson methods instead of rapidjson.
Changed the JSON parsing to use simdjson DOM API.
The "RunCommand" function is updated to read commands differently as simdjson returns a std::string_view+simdjson::dom::object instead of a rapidjson object.
In general, the patch seems to be part of a broader refactoring effort aimed at transition from rapidjson to simdjson for JSON processing.
Hello, I am a code review bot on flows.network. Here are my reviews of changed source code files in this PR.
test/spec/CMakeLists.txt
The provided code in general looks well-structured and following good coding practices for a CMake file. Below are a few notes:
Assumptions about External Packages: The code assumes the availability of the 'simdjson' package. If it is not found, then it fetches the content from GIT_REPOSITORY. That's a good thing, but it also assumes that FetchContent is available with the inclusion command 'include(FetchContent)' and that the other packages like GTEST_BOTH_LIBRARIES and wasmedgeCommon are available when linking libraries. It's better to include checks for these assumptions or provide informative error messages if they aren't met.
MSVC Specifics: There is a specific section for handling MSVC (Microsoft Visual Studio Compiler). If the compiler is not MSVC, the code doesn't perform any operations to handle other compilers like GCC or Clang. It's better to add some error handling or exception handling for other compilers, or at least a message indicating that the required operations are only intended for MSVC.
Hardcoded Library Versions: Versions for the external Git repositories (like 'GIT_TAG wasm-dev-0.13.0' and 'GIT_TAG tags/v3.2.1') are hardcoded. Hardcoding versions is a common practice, but it could lead to outdated versions over time and potentially complicate version management.
Ignored Compiler Warnings: The code ignores a range of compiler warnings for MSVC. This can be helpful for suppressing unnecessary or non-problematic warnings but ignoring too many warnings can lead to overlooking real issues within the code.
Avoid Silent Errors: The failure of the function 'wasmedge_copy_spec_testsuite()' is not checked. If the directories or files cannot be copied for some reason (e.g., lack of permissions, disk space, etc.), the script will not fail and will continue executing the rest of the code, which could inadvertently cause a silent error.
Remember that these comments are based purely on standard good practices and may not apply depending on the specifics of the project, the development environment, and the team's coding conventions.
The patch summarises some key changes in the code:
Library Change: It changes the JSON parsing library from RapidJSON to SIMDJSON. This impacts several subsequent code sections, from the GIT repository the code is fetched from, to the library specific flags used in the configuration.
Changes in the Compiler Flags: Certain compiler warning flags specific to RapidJSON have been removed, and several SIMDJSON compiler warning flags have been added. These flags are specific to the Microsoft Visual Studio Compiler (MSVC).
Static Library Configuration: For SIMDJSON library, two options
SIMDJSON_JUST_LIBRARYandSIMDJSON_BUILD_STATICare set toONviaCACHE INTERNAL. This indicates that SIMDJSON should be built as a static library and just SIMDJSON, nothing else.Linking Library Change: In the
target_link_librariesfunction, the library name has been changed fromRapidJSONtosimdjson. This aligns with the earlier change in adopted library.These changes primarily reflect a decision to change the JSON library in use from RapidJSON to SIMDJSON, with a number of necessary adjustments throughout the code to ensure compatibility.
test/spec/spectest.cpp
Here are some potential issues that I found:
Missing include guard or
#pragma once: Including the same header multiple times can lead to redefinitions or multiple inclusions. To protect your headers, use include guard or#pragma once.Converting between string view and string: Conversions between
std::string_viewandstd::stringare made without checking for null termination. This could potentially introduce bugs.Lack of error handling: There are several places where functions are called but it is assumed that they will always succeed. Adding proper error handling will make the code more robust, such as
Doc = parser.load(FName);,Args = Action["args"];, etc.Unexpected behavior:
assumingUnreachable();function is called without any apparent check. If this function is supposed to represent an error or abnormal condition, there should be a condition that triggers its call. A simple unreachable code is a smell of a deeper problem.Using
strcmpandstrstrfunctions from the C library can produce undefined behavior if the strings are not null-terminated. Consider using the equivalentstd::string::compareandstd::string::findfunctions instead.If the
spdlog::errorfunction does not stop the execution of the program, thestringContainsfunction may introduce a silent fail, misleading the developer about the real status of the execution. If it is a critical problem, consider throwing an exception or stopping the execution.Magic numbers: Avoid using magic constant numbers in your code. Use named constant variables instead for better readability.
Missing comments: Although the code is mostly well-written and clean, more comments could help readers understand the code easier.
Check the use of reinterpret_cast. It can be risky if not properly used, remember to ensure you're interpreting the data correctly.
Be careful when using
std::map::operator[]as it will silently create a new entry in the map, if the key does not already exist.Additionally, it would be recommended to follow a consistent curly braces {} style for conditional and control structures. In this piece of code, sometimes they're on the same line and sometimes on a new line.
This patch appears to modify a software testing file and includes several significant changes. The primary objective of these changes appears to focus on replacing the usage of rapidjson with simdjson in various places.
The updates include:
Removal of #define flags related to RAPIDJSON_HAS_STDSTRING, RAPIDJSON_SSE42, RAPIDJSON_SSE2, and RAPIDJSON_NEON replacing rapidjson includes with simdjson.
Modification of several helper functions including
GetModuleName, Invoke, Get, TrapInvoke, RunCommand,etc with simdjson methods instead of rapidjson.Changed the JSON parsing to use simdjson DOM API.
The "RunCommand" function is updated to read commands differently as simdjson returns a std::string_view+simdjson::dom::object instead of a rapidjson object.
In general, the patch seems to be part of a broader refactoring effort aimed at transition from rapidjson to simdjson for JSON processing.
cc https://github.com/WasmEdge/WasmEdge/pull/2659