Copilot commented on code in PR #7538:
URL: https://github.com/apache/ignite-3/pull/7538#discussion_r2799024268
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
+ else()
+ message(STATUS "clang-tidy not found!")
+ endif()
+endif()
+
+if(ENABLE_CPPCHECK)
+ find_program(CPPCHECK_BIN cppcheck REQUIRED)
+ if(CPPCHECK_BIN)
+ message(STATUS "cppcheck: ${CPPCHECK_BIN}")
+ message(STATUS "Please do not use parallel build with cppcheck
enabled! Cppcheck writes results to the file and result might be corrupted in
case of parallel build.")
+ set(CMAKE_CXX_CPPCHECK ${CPPCHECK_BIN} --enable=all --xml
--xml-version=3 --output-file=cppcheck_report.xml)
+ add_custom_target(cppcheck_html_report ALL
+ COMMAND cppcheck-htmlreport
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/tuple/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/protocol/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/common/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/network/cppcheck_report.xml
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/client-test/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/fake_server/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/odbc-test/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/test-common/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_ODBC}>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/odbc/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_CLIENT}>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/client/cppcheck_report.xml>
+ --report-dir=${CMAKE_CURRENT_BINARY_DIR}/cppcheck_report
+ DEPENDS
+ ignite-tuple
+ ignite-protocol
+ ignite-common
+ ignite-network
+ $<$<BOOL:${ENABLE_TESTS}>:ignite-client-test>
+ $<$<BOOL:${ENABLE_TESTS}>:ignite-fake-server>
+ $<$<BOOL:${ENABLE_TESTS}>:ignite-odbc-test>
+ $<$<BOOL:${ENABLE_TESTS}>:ignite-test-common>
+ $<$<BOOL:${ENABLE_ODBC}>:ignite3-odbc>
+ $<$<BOOL:${ENABLE_CLIENT}>:ignite3-client>
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+ VERBATIM
+ )
+ else()
+ message(STATUS "cppcheck not found!")
+ endif()
+endif()
Review Comment:
Similar to the clang-tidy integration, the error handling here is
inconsistent. The find_program uses REQUIRED which will cause a FATAL_ERROR if
cppcheck is not found, making the else block at lines 115-117 unreachable code.
For consistency with the IWYU pattern (lines 59-66), either: (a) remove
REQUIRED and add proper error handling with FATAL_ERROR in the else block, or
(b) remove the unreachable else block entirely if REQUIRED is kept.
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
+ else()
+ message(STATUS "clang-tidy not found!")
+ endif()
+endif()
+
+if(ENABLE_CPPCHECK)
+ find_program(CPPCHECK_BIN cppcheck REQUIRED)
+ if(CPPCHECK_BIN)
+ message(STATUS "cppcheck: ${CPPCHECK_BIN}")
+ message(STATUS "Please do not use parallel build with cppcheck
enabled! Cppcheck writes results to the file and result might be corrupted in
case of parallel build.")
+ set(CMAKE_CXX_CPPCHECK ${CPPCHECK_BIN} --enable=all --xml
--xml-version=3 --output-file=cppcheck_report.xml)
+ add_custom_target(cppcheck_html_report ALL
+ COMMAND cppcheck-htmlreport
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/tuple/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/protocol/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/common/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/network/cppcheck_report.xml
Review Comment:
The cppcheck HTML report generation references XML files for protocol and
network modules (lines 91-93) which are only built when ENABLE_CLIENT,
ENABLE_ODBC, or ENABLE_PROTOCOL is enabled (see line 197 of CMakeLists.txt).
However, these --file arguments don't use generator expressions to
conditionally include them.
When none of these options are enabled, the protocol and network modules
won't be built, their cppcheck_report.xml files won't exist, but the
cppcheck-htmlreport command will still try to process them and fail.
Wrap these --file arguments in generator expressions similar to how it's
done for tests and optional modules on lines 94-99.
```suggestion
$<$<OR:$<BOOL:${ENABLE_CLIENT}>,$<BOOL:${ENABLE_ODBC}>,$<BOOL:${ENABLE_PROTOCOL}>>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/protocol/cppcheck_report.xml>
--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/common/cppcheck_report.xml
$<$<OR:$<BOOL:${ENABLE_CLIENT}>,$<BOOL:${ENABLE_ODBC}>,$<BOOL:${ENABLE_PROTOCOL}>>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/network/cppcheck_report.xml>
```
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
Review Comment:
The variable CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR is set but CMake doesn't
have a built-in variable with this exact name. The correct variable name for
exporting clang-tidy fixes is CMAKE_<LANG>_CLANG_TIDY_EXPORT_FIXES_DIR,
and it's typically not set directly like this.
If the intent is to export fixes to a directory, the --export-fixes option
should be added to the CMAKE_CXX_CLANG_TIDY command list instead. For example,
add "--export-fixes-dir=clang-tidy-reports-dir" to the CMAKE_CXX_CLANG_TIDY
list on line 72.
```suggestion
"--exclude-header-filter=\"^.*(build|tests).*\"gm"
"--export-fixes-dir=clang-tidy-reports-dir")
```
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
+ else()
+ message(STATUS "clang-tidy not found!")
+ endif()
+endif()
+
+if(ENABLE_CPPCHECK)
+ find_program(CPPCHECK_BIN cppcheck REQUIRED)
+ if(CPPCHECK_BIN)
+ message(STATUS "cppcheck: ${CPPCHECK_BIN}")
+ message(STATUS "Please do not use parallel build with cppcheck
enabled! Cppcheck writes results to the file and result might be corrupted in
case of parallel build.")
+ set(CMAKE_CXX_CPPCHECK ${CPPCHECK_BIN} --enable=all --xml
--xml-version=3 --output-file=cppcheck_report.xml)
Review Comment:
The cppcheck command uses --output-file=cppcheck_report.xml which writes to
a single file, but this will cause issues because each module (tuple, protocol,
common, network, client, odbc, tests) will overwrite the same file. The HTML
report generation then expects separate XML files per module (e.g.,
ignite/tuple/cppcheck_report.xml).
The --output-file option should be removed from CMAKE_CXX_CPPCHECK so that
cppcheck outputs to stdout/stderr as usual, and each module's output can be
captured separately. Alternatively, if XML output is needed, each target should
configure its own output file path, but this requires per-target configuration
rather than a global CMAKE_CXX_CPPCHECK setting.
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
+ else()
+ message(STATUS "clang-tidy not found!")
+ endif()
+endif()
+
+if(ENABLE_CPPCHECK)
+ find_program(CPPCHECK_BIN cppcheck REQUIRED)
+ if(CPPCHECK_BIN)
+ message(STATUS "cppcheck: ${CPPCHECK_BIN}")
+ message(STATUS "Please do not use parallel build with cppcheck
enabled! Cppcheck writes results to the file and result might be corrupted in
case of parallel build.")
+ set(CMAKE_CXX_CPPCHECK ${CPPCHECK_BIN} --enable=all --xml
--xml-version=3 --output-file=cppcheck_report.xml)
+ add_custom_target(cppcheck_html_report ALL
+ COMMAND cppcheck-htmlreport
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/tuple/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/protocol/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/common/cppcheck_report.xml
+ --file
${CMAKE_CURRENT_BINARY_DIR}/ignite/network/cppcheck_report.xml
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/client-test/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/fake_server/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/odbc-test/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_TESTS}>:--file
${CMAKE_CURRENT_BINARY_DIR}/tests/test-common/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_ODBC}>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/odbc/cppcheck_report.xml>
+ $<$<BOOL:${ENABLE_CLIENT}>:--file
${CMAKE_CURRENT_BINARY_DIR}/ignite/client/cppcheck_report.xml>
+ --report-dir=${CMAKE_CURRENT_BINARY_DIR}/cppcheck_report
+ DEPENDS
+ ignite-tuple
+ ignite-protocol
+ ignite-common
+ ignite-network
Review Comment:
The DEPENDS list for the cppcheck_html_report target includes protocol and
network as unconditional dependencies (lines 103-105), but these targets are
only added when ENABLE_CLIENT, ENABLE_ODBC, or ENABLE_PROTOCOL is enabled (see
line 197 of CMakeLists.txt). This will cause a CMake configuration error when
all three options are disabled.
Wrap these dependencies in generator expressions like the other conditional
dependencies. For example:
$<$<BOOL:${ENABLE_CLIENT_OR_ODBC_OR_PROTOCOL}>:ignite-protocol> or
create a condition that checks if any of these options are enabled.
```suggestion
$<$<OR:$<BOOL:${ENABLE_CLIENT}>,$<OR:$<BOOL:${ENABLE_ODBC}>,$<BOOL:${ENABLE_PROTOCOL}>>>:ignite-protocol>
ignite-common
$<$<OR:$<BOOL:${ENABLE_CLIENT}>,$<OR:$<BOOL:${ENABLE_ODBC}>,$<BOOL:${ENABLE_PROTOCOL}>>>:ignite-network>
```
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
+ set(CMAKE_CXX_CLANG_TIDY_EXPORT_FIXES_DIR "clang-tidy-reports-dir")
+ else()
+ message(STATUS "clang-tidy not found!")
+ endif()
+endif()
Review Comment:
The error handling for clang-tidy is inconsistent with the existing IWYU
integration pattern. When IWYU is enabled but not found (lines 59-66), it uses
FATAL_ERROR to stop the build. However, clang-tidy uses REQUIRED in
find_program but then falls back to a STATUS message if not found. This creates
two issues:
1. The REQUIRED flag will already cause a FATAL_ERROR if clang-tidy is not
found, making the else block at lines 77-79 unreachable
2. The error handling behavior is inconsistent with the IWYU pattern
Consider either: (a) using FATAL_ERROR like IWYU for consistency, or (b)
removing REQUIRED and properly handling the case when the tool is not found
with FATAL_ERROR.
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
Review Comment:
The clang-tidy checks are hardcoded in CMakeLists.txt, but there's already a
comprehensive .clang-tidy configuration file at the project root
(modules/platforms/cpp/.clang-tidy). Having duplicate check configurations in
two places creates a maintenance burden and potential inconsistencies.
Consider removing the --checks flag from line 73 to let clang-tidy use the
existing .clang-tidy configuration file, which already specifies a detailed
list of enabled checks. This follows the principle of single source of truth
and is the standard practice when a .clang-tidy file exists.
```suggestion
```
##########
modules/platforms/cpp/CMakeLists.txt:
##########
@@ -63,6 +65,58 @@ if(ENABLE_IWYU)
set(CMAKE_C_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
endif()
+if(ENABLE_CLANG_TIDY)
+ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
+ if(CLANG_TIDY_BIN)
+ message(STATUS "clang-tidy: ${CLANG_TIDY_BIN}")
+ set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_BIN}
+
"--checks=-*,clang-analyzer-*,clang-analyzer-cplusplus*,bugprone*,concurrency-*,cppcoreguidelines-*,performance-*,portability-*,readability-*"
+
"--header-filter=\"^.*(client|common|network|odbc|protocol|tuple).*\"gm"
+ "--exclude-header-filter=\"^.*(build|tests).*\"gm")
Review Comment:
The header filter regex patterns have incorrect syntax. The patterns end
with "gm" flags but CMake/clang-tidy don't support these PCRE-style flags in
this context. The 'g' (global) flag is implicit in clang-tidy header filters,
and 'm' (multiline) is not relevant for file path matching.
The correct patterns should be:
- "--header-filter=^.*(client|common|network|odbc|protocol|tuple).*"
- "--exclude-header-filter=^.*(build|tests).*"
Remove the "gm" suffix and the extra escaping of the quotes.
```suggestion
"--header-filter=^.*(client|common|network|odbc|protocol|tuple).*"
"--exclude-header-filter=^.*(build|tests).*")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
