Improvements on cppcheck #22

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@eile
Contributor
eile commented Jan 27, 2014

No description provided.

@rpavlik rpavlik commented on the diff Apr 15, 2015
CppcheckTargets.cmake
endif()
-if(CPPCHECK_FOUND)
- if(NOT TARGET all_cppcheck)
- add_custom_target(all_cppcheck)
- set_target_properties(all_cppcheck PROPERTIES EXCLUDE_FROM_ALL TRUE)
- endif()
+if(NOT CPPCHECK_FOUND)
+ add_custom_target(cppcheck
+ COMMENT "cppcheck executable not found")
@rpavlik
rpavlik Apr 15, 2015 Owner

Does this actually work the way I'd expect by reading it? Does it result in a build success or a build failure?

@eile
eile Apr 17, 2015 Contributor

It will print the message and continue without failure.

@rpavlik rpavlik commented on the diff Apr 15, 2015
CppcheckTargets.cmake
@@ -27,19 +27,29 @@ endif()
set(__add_cppcheck YES)
if(NOT CPPCHECK_FOUND)
- find_package(cppcheck QUIET)
+ find_package(cppcheck)
@rpavlik
rpavlik Apr 15, 2015 Owner

I was under the impression that if you had to search for a dependent package, it was polite to be quiet, or at least as quiet as you were asked to be, though I have found that's not always what feels like the right solution so maybe it merits changing.

@rpavlik rpavlik commented on the diff Apr 15, 2015
CppcheckTargets.cmake
endif()
function(add_cppcheck_sources _targetname)
if(CPPCHECK_FOUND)
- set(_cppcheck_args)
+ set(_cppcheck_args --force -I ${CMAKE_SOURCE_DIR}
@rpavlik
rpavlik Apr 15, 2015 Owner

Any particular reason why those args are being forced? I can imagine that the error one is for a proper exit to the build, and the inline-suppr and unmatchedsuppression are for code-embedded suppressions and compatibility with versions that don't support the same suppressions. The automatic include path seems a bit presumptuous, though maybe not. I don't remember offhand what --force does.

@rpavlik rpavlik commented on the diff Apr 15, 2015
CppcheckTargets.cmake
@@ -113,9 +123,7 @@ function(add_cppcheck_sources _targetname)
FAIL_REGULAR_EXPRESSION
"${CPPCHECK_FAIL_REGULAR_EXPRESSION}")
- add_custom_command(TARGET
- all_cppcheck
- PRE_BUILD
+ add_custom_target(${_targetname}_cppcheck
@rpavlik
rpavlik Apr 15, 2015 Owner

Thanks - this setup of having multiple targets seems like a much more reliable solution than pre-build commands on one target.

@rpavlik rpavlik commented on the diff Apr 15, 2015
CppcheckTargets.cmake
@@ -136,7 +145,9 @@ function(add_cppcheck _name)
"add_cppcheck given a target name that does not exist: '${_name}' !")
endif()
if(CPPCHECK_FOUND)
- set(_cppcheck_args)
@rpavlik
rpavlik Apr 15, 2015 Owner

This looks like the patch above, which probably means this code could use some refactoring to reduce duplication.

@rpavlik rpavlik commented on the diff Apr 15, 2015
Findcppcheck.cmake
@@ -149,6 +150,11 @@ endif()
set(CPPCHECK_ALL
"${CPPCHECK_EXECUTABLE} ${CPPCHECK_POSSIBLEERROR_ARG} ${CPPCHECK_UNUSEDFUNC_ARG} ${CPPCHECK_STYLE_ARG} ${CPPCHECK_QUIET_ARG} ${CPPCHECK_INCLUDEPATH_ARG} some/include/path")
+execute_process(COMMAND "${CPPCHECK_EXECUTABLE}" --version
@rpavlik
rpavlik Apr 15, 2015 Owner

Should this be guarded with something like if(CPPCHECK_EXECUTABLE)?

@eile eile closed this Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment