Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a codecheck make target #682

Merged
merged 3 commits into from
Sep 2, 2021
Merged

Add a codecheck make target #682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b993159
Add a codecheck make target
azeey Aug 31, 2021
55dc99b
Merge branch 'sdf9' into sdf9_codecheck_target
scpeters Sep 2, 2021
9a6d507
Remove version requirement when finding ignition-cmake2
azeey Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,28 @@ else (build_errors)
add_subdirectory(doc)
endif(BUILD_SDF)

########################################
# Setup Codecheck
include (IgnCodeCheck)
set(CPPCHECK_DIRS
${PROJECT_SOURCE_DIR}/src
${PROJECT_SOURCE_DIR}/include
${PROJECT_SOURCE_DIR}/test/integration
${PROJECT_SOURCE_DIR}/test/performance)

set(CPPCHECK_INCLUDE_DIRS
${PROJECT_BINARY_DIR}
${PROJECT_SOURCE_DIR}/include
${PROJECT_SOURCE_DIR}/test/integration
${PROJECT_SOURCE_DIR}/test/performance)

# Ignore vendored directories.
file(WRITE ${PROJECT_BINARY_DIR}/cppcheck.suppress
"*:${PROJECT_SOURCE_DIR}/src/win/*\n"
"*:${PROJECT_SOURCE_DIR}/src/urdf/*\n"
)
ign_setup_target_for_codecheck()

########################################
# Make the package config file
configure_file(${CMAKE_SOURCE_DIR}/cmake/sdformat_pc.in
Expand Down
7 changes: 7 additions & 0 deletions cmake/SearchForStuff.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ macro (check_gcc_visibility)
check_cxx_compiler_flag(-fvisibility=hidden GCC_SUPPORTS_VISIBILITY)
endmacro()

########################################
# Find ignition cmake2
# Only for using the testing macros and creating the codecheck target, not
# really being use to configure the whole project
find_package(ignition-cmake2 2.3 REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason why version 2.3 is the minimum required version? It looks like ign_setup_target_for_codecheck exists before 2.3: https://github.com/ignitionrobotics/ign-cmake/blob/ignition-cmake2_2.2.0/cmake/IgnCodeCheck.cmake#L1-L2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will actually need to be updated since this PR needs a new release of ign-cmake to ensure make codecheck is clean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't add that as a version requirement, since that will cause build failures if people have an outdated version of ign-cmake, which is a bit extreme to ensure correct linting. I would recommend a find_package call with no version requirement, since IgnCodeCheck was present in ign-cmake's 2.0.0 release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point; I think I agree with @scpeters comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. If it won't cause failure in CI, since we assume CI will have the latest ign-cmake, we can remove the version requirement.

Also, I originally thought gazebosim/gz-cmake#187 would be needed to make CI pass, but it turns out without that PR, we do get output from codechech, but they are just warnings, not errors. So technically, this PR can go in without the ign-cmake release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9a6d507

set(IGN_CMAKE_VER ${ignition-cmake2_VERSION_MAJOR})

########################################
# Find ignition math
# Set a variable for generating ProjectConfig.cmake
Expand Down
2 changes: 1 addition & 1 deletion include/sdf/Console.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ namespace sdf
/// \brief Constructor.
/// \param[in] _stream Pointer to an output stream operator. Can be
/// NULL/nullptr.
public: ConsoleStream(std::ostream *_stream) :
public: explicit ConsoleStream(std::ostream *_stream) :
stream(_stream) {}

/// \brief Redirect whatever is passed in to both our ostream
Expand Down
4 changes: 4 additions & 0 deletions include/sdf/Exception.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ namespace sdf
/// \param[in] _line Line number where the error occurred
/// \param[in] _msg Error message
public: InternalError(const char *_file, std::int64_t _line,
// cppcheck-suppress passedByValue
const std::string _msg);

/// \brief Destructor
Expand All @@ -148,8 +149,11 @@ namespace sdf
/// \param[in] _msg Function where assertion failed
public: AssertionInternalError(const char *_file,
std::int64_t _line,
// cppcheck-suppress passedByValue
const std::string _expr,
// cppcheck-suppress passedByValue
const std::string _function,
// cppcheck-suppress passedByValue
const std::string _msg = "");
/// \brief Destructor
public: virtual ~AssertionInternalError();
Expand Down
1 change: 1 addition & 0 deletions include/sdf/Param.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ namespace sdf
template<typename T>
std::string ParamPrivate::TypeToString() const
{
// cppcheck-suppress syntaxError
if constexpr (std::is_same_v<T, bool>)
return "bool";
else if constexpr (std::is_same_v<T, char>)
Expand Down
4 changes: 4 additions & 0 deletions include/sdf/parser_urdf.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,13 @@ namespace sdf
private: void ParseSDFExtension(TiXmlDocument &_urdfXml);

/// list extensions for debugging
// cppcheck-suppress unusedPrivateFunction
// cppcheck-suppress unmatchedSuppression
private: void ListSDFExtensions();

/// list extensions for debugging
// cppcheck-suppress unusedPrivateFunction
// cppcheck-suppress unmatchedSuppression
private: void ListSDFExtensions(const std::string &_reference);
};
}
Expand Down
1 change: 0 additions & 1 deletion src/Error.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ namespace sdf
inline namespace SDF_VERSION_NAMESPACE {

/////////////////////////////////////////////////
// cppcheck-suppress unusedFunction
std::ostream &operator<<(std::ostream &_out, const sdf::Error &_err)
{
_out << "Error Code "
Expand Down
2 changes: 0 additions & 2 deletions src/SDF.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ static std::function<std::string(const std::string &)> g_findFileCB;
std::string SDF::version = SDF_VERSION; // NOLINT(runtime/string)

/////////////////////////////////////////////////
// cppcheck-suppress passedByValue
void setFindCallback(std::function<std::string(const std::string &)> _cb)
{
g_findFileCB = _cb;
Expand All @@ -69,7 +68,6 @@ std::string findFile(const std::string &_filename, bool _searchLocalPath,
{
// Check to see if the URI in the global map is the first part of the
// given filename
// cppcheck-suppress stlIfStrFind
if (_filename.find(iter->first) == 0)
{
std::string suffix = _filename;
Expand Down
4 changes: 0 additions & 4 deletions src/ign.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include "ign.hh"

//////////////////////////////////////////////////
// cppcheck-suppress unusedFunction
extern "C" SDFORMAT_VISIBLE int cmdCheck(const char *_path)
{
int result = 0;
Expand Down Expand Up @@ -96,7 +95,6 @@ extern "C" SDFORMAT_VISIBLE int cmdCheck(const char *_path)
}

//////////////////////////////////////////////////
// cppcheck-suppress unusedFunction
extern "C" SDFORMAT_VISIBLE char *ignitionVersion()
{
#ifdef _MSC_VER
Expand All @@ -109,7 +107,6 @@ extern "C" SDFORMAT_VISIBLE char *ignitionVersion()
//////////////////////////////////////////////////
/// \brief Print the full description of the SDF spec.
/// \return 0 on success, -1 if SDF could not be initialized.
// cppcheck-suppress unusedFunction
extern "C" SDFORMAT_VISIBLE int cmdDescribe(const char *_version)
{
sdf::SDFPtr sdf(new sdf::SDF());
Expand All @@ -130,7 +127,6 @@ extern "C" SDFORMAT_VISIBLE int cmdDescribe(const char *_version)
}

//////////////////////////////////////////////////
// cppcheck-suppress unusedFunction
extern "C" SDFORMAT_VISIBLE int cmdPrint(const char *_path)
{
if (!sdf::filesystem::exists(_path))
Expand Down
1 change: 0 additions & 1 deletion test/integration/locale_fix_cxx.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ TEST(CheckFixForLocal, CheckFixForCxxLocal)
{
struct CommaDecimalPointFacet : std::numpunct<char>
{
// cppcheck-suppress unusedFunction
char do_decimal_point() const
{
return ',';
Expand Down