WIP: Configure clang-tidy CI check #4022

Draft
SiarheiFedartsou wants to merge 1 commit from SiarheiFedartsou/sf-clang-tidy2 into master
16 changed files with 153 additions and 14 deletions

114
.clang-tidy Normal file
View file

@ -0,0 +1,114 @@
---
Checks: >
SiarheiFedartsou commented 2022-12-04 12:22:54 +00:00 (Migrated from github.com)
Review

Many of these checks are really useful, but I disabled those which were non trivial to resolve at the moment, but I propose to resolve them afterwards iteratively in separate PRs.

Many of these checks are really useful, but I disabled those which were non trivial to resolve at the moment, but I propose to resolve them afterwards iteratively in separate PRs.
biodranik commented 2022-12-04 12:48:05 +00:00 (Migrated from github.com)
Review

Let's focus on important checks/fixes that may break the production.

Let's focus on _important_ checks/fixes that may break the production.
SiarheiFedartsou commented 2022-12-04 13:47:25 +00:00 (Migrated from github.com)
Review

Do you propose to fix them as part of this PR? Or it is just an advice for future PRs?

Do you propose to fix them as part of this PR? Or it is just an advice for future PRs?
biodranik commented 2022-12-04 14:20:18 +00:00 (Migrated from github.com)
Review

Most of the straight-forward fixes can be merged in a separate PR. Then this one can be rebased.

Most of the straight-forward fixes can be merged in a separate PR. Then this one can be rebased.
bugprone-*,
-bugprone-narrowing-conversions,
-bugprone-easily-swappable-parameters,
-bugprone-branch-clone,
-bugprone-misplaced-widening-cast,
-bugprone-exception-escape,
-bugprone-implicit-widening-of-multiplication-result,
-bugprone-integer-division,
-bugprone-reserved-identifier,
-bugprone-unhandled-self-assignment,
-bugprone-forward-declaration-namespace,
-bugprone-sizeof-expression,
-bugprone-throw-keyword-missing,
-bugprone-macro-parentheses,
-bugprone-forwarding-reference-overload,
-bugprone-move-forwarding-reference,
-bugprone-incorrect-roundings,
-bugprone-unchecked-optional-access,
-bugprone-assignment-in-if-condition,
-bugprone-signed-char-misuse,
-bugprone-suspicious-include,
-bugprone-sizeof-container,
-bugprone-bool-pointer-implicit-conversion,
-bugprone-lambda-function-name,
-bugprone-use-after-move,
-bugprone-suspicious-enum-usage,
-clang-analyzer-*,
-clang-diagnostic-deprecated-declarations,
-clang-diagnostic-constant-conversion,
-clang-diagnostic-unqualified-std-cast-call,
google-*,
-google-build-explicit-make-pair,
-google-build-using-namespace,
-google-explicit-constructor,
-google-default-arguments,
-google-readability-braces-around-statements,
-google-readability-casting,
-google-readability-namespace-comments,
-google-readability-function,
-google-readability-todo,
-google-runtime-int,
-google-build-namespaces,
-google-runtime-references,
-google-readability-function-size,
-google-global-names-in-headers,
llvm-*,
-llvm-namespace-comment,
-llvm-qualified-auto,
-llvm-include-order,
-llvm-else-after-return,
-llvm-header-guard,
-llvm-twine-local,
misc-*,
-misc-argument-comment,
-misc-const-correctness,
-misc-non-private-member-variables-in-classes,
-misc-unconventional-assign-operator,
-misc-no-recursion,
-misc-misplaced-const,
-misc-definitions-in-headers,
-misc-unused-parameters,
-misc-unused-using-decls,
-misc-static-assert,
-misc-confusable-identifiers,
-misc-redundant-expression,
performance-*,
-performance-noexcept-move-constructor,
-performance-no-int-to-ptr,
-performance-unnecessary-value-param,
-performance-unnecessary-copy-initialization,
-performance-move-const-arg,
-performance-inefficient-string-concatenation,
-performance-no-automatic-move,
-performance-type-promotion-in-math-fn,
-performance-for-range-copy,
readability-*,
-readability-avoid-const-params-in-decls,
-readability-braces-around-statements,
-readability-container-size-empty,
-readability-convert-member-functions-to-static,
-readability-container-data-pointer,
-readability-const-return-type,
-readability-function-cognitive-complexity,
-readability-function-size,
-readability-identifier-naming,
-readability-implicit-bool-conversion,
-readability-magic-numbers,
-readability-else-after-return,
-readability-inconsistent-declaration-parameter-name,
-readability-isolate-declaration,
-readability-identifier-length,
-readability-redundant-declaration,
-readability-uppercase-literal-suffix,
-readability-named-parameter,
-readability-qualified-auto,
-readability-suspicious-call-argument,
-readability-redundant-access-specifiers,
-readability-redundant-member-init,
-readability-static-definition-in-anonymous-namespace,
-readability-use-anyofallof,
-readability-simplify-boolean-expr,
-readability-make-member-function-const,
-readability-redundant-string-init,
-readability-non-const-parameter,
-readability-static-accessed-through-instance,
-readability-misleading-indentation,
-readability-redundant-string-cstr,
-readability-redundant-smartptr-get,
-readability-duplicate-include
WarningsAsErrors: '*'
HeaderFilterRegex: '.*'

View file

@ -63,7 +63,7 @@ jobs:
CXX: clang++-14
# -g1 should slightly reduce build time.
run: |
cmake . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=-g1 -DUNITY_DISABLE=ON
cmake . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=-g1 -DUNITY_DISABLE=ON -DUSE_CLANG_TIDY=ON
SiarheiFedartsou commented 2022-12-04 12:26:24 +00:00 (Migrated from github.com)
Review

It was just most "convenient" place in current CI configuration to put this flag, but it is not a big deal to do it in another way. We can:

  • Extract it to separate job
  • Move to some another job
  • Leave it here
  • ...

Let's just discuss, I am not sure what is the best solution.

It was just most "convenient" place in current CI configuration to put this flag, but it is not a big deal to do it in another way. We can: - Extract it to separate job - Move to some another job - Leave it here - ... Let's just discuss, I am not sure what is the best solution.
biodranik commented 2022-12-04 12:49:57 +00:00 (Migrated from github.com)
Review

How does clang-tidy check work in reality? Can it be run independently on its own, without building the whole project? If yes, then it would be better to use a separate job in the same check action for it to differentiate build errors vs tidy warnings.

How does clang-tidy check work in reality? Can it be run independently on its own, without building the whole project? If yes, then it would be better to use a separate job in the same check action for it to differentiate build errors vs tidy warnings.
Ferenc- commented 2022-12-04 20:37:57 +00:00 (Migrated from github.com)
Review

Can it be run independently on its own, without building the whole project?

Yes, but it needs an up to date compile command database, where for each and every compilation unit, the info about how exactly to build is stored. Like where the includes are coming from, what is defined etc.
And therfore it is typical, to run it together with the compilation, so the result is surely not out of date.

BTW, if we are into static checking, then there is this frontend, which next to clang-tidy could do even more, if needed..

> Can it be run independently on its own, without building the whole project? Yes, but it needs an up to date compile command database, where for each and every compilation unit, the info about how exactly to build is stored. Like where the includes are coming from, what is defined etc. And therfore it is typical, to run it together with the compilation, so the result is surely not out of date. BTW, if we are into static checking, then there is [this frontend](https://github.com/Ericsson/codechecker#command-line-cc-analysis), which next to `clang-tidy` could do even more, if needed..
SiarheiFedartsou commented 2022-12-04 21:07:06 +00:00 (Migrated from github.com)
Review

@Ferenc- if we run it independently is there good way to control for which targets we run it and for which not? E.g. I don’t think we want to run it for third party dependencies and CXX_CLANG_TIDY allows to control it quite easily(we just don’t set it for targets we don’t want to run clang-tidy for and that’s it)

@Ferenc- if we run it independently is there good way to control for which targets we run it and for which not? E.g. I don’t think we want to run it for third party dependencies and CXX_CLANG_TIDY allows to control it quite easily(we just don’t set it for targets we don’t want to run clang-tidy for and that’s it)
biodranik commented 2022-12-04 22:56:57 +00:00 (Migrated from github.com)
Review

@Ferenc- did you use codechecker before? Does it integrate easily with CMake?
Can't compile command database be generated without building the binaries? My main point is to do as fast check stage as possible, without unnecessary build overhead.

@Ferenc- did you use codechecker before? Does it integrate easily with CMake? Can't compile command database be generated _without_ building the binaries? My main point is to do as fast check stage as possible, without unnecessary build overhead.
SiarheiFedartsou commented 2022-12-05 08:31:50 +00:00 (Migrated from github.com)
Review

It seems we can just set this variable and it will output this database right after CMake https://cmake.org/cmake/help/latest/variable/CMAKE_EXPORT_COMPILE_COMMANDS.html (but it is to be checked - will do when will have a time)

It seems we can just set this variable and it will output this database right after CMake https://cmake.org/cmake/help/latest/variable/CMAKE_EXPORT_COMPILE_COMMANDS.html (but it is to be checked - will do when will have a time)
- name: Compile
shell: bash

View file

@ -126,6 +126,7 @@ if (BUILD_DESIGNER)
add_definitions(-DBUILD_DESIGNER)
endif()
option(USE_CLANG_TIDY "Enable Clang Tidy" OFF)
option(USE_ASAN "Enable Address Sanitizer" OFF)
option(USE_TSAN "Enable Thread Sanitizer" OFF)
option(USE_LIBFUZZER "Enable LibFuzzer" OFF)
@ -161,6 +162,10 @@ if (PLATFORM_LINUX)
option(USE_PPROF "Enable Google Profiler" OFF)
endif()
if (USE_CLANG_TIDY)
message(STATUS "Clang Tidy is enabled")
endif()
if (USE_ASAN)
message(STATUS "Address Sanitizer is enabled")
endif()

View file

@ -74,7 +74,7 @@ public:
buffer_vector(buffer_vector const &) = default;
buffer_vector(buffer_vector && rhs) : m_size(rhs.m_size), m_dynamic(move(rhs.m_dynamic))
buffer_vector(buffer_vector && rhs) : m_size(rhs.m_size), m_dynamic(std::move(rhs.m_dynamic))
{
if (!IsDynamic())
MoveStatic(rhs);

View file

@ -18,7 +18,7 @@ DeferredTask::DeferredTask(Duration const & duration) : m_duration(duration)
if (m_cv.wait_for(l, m_duration) != std::cv_status::timeout || !m_fn)
continue;
auto fn = move(m_fn);
auto fn = std::move(m_fn);
m_fn = nullptr;
l.unlock();

View file

@ -33,7 +33,7 @@ function(omim_add_executable executable)
# Enable warnings for all our binaries.
target_compile_options(${executable} PRIVATE ${OMIM_WARNING_FLAGS})
target_include_directories(${executable} PRIVATE ${OMIM_INCLUDE_DIRS})
target_include_directories(${executable} SYSTEM PRIVATE ${OMIM_INCLUDE_DIRS})
if (USE_ASAN)
biodranik commented 2022-12-04 13:06:18 +00:00 (Migrated from github.com)
Review

Generally, it doesn't look right to mark some 3p lib includes as system ones. Are there any possible side effects from this change?

Generally, it doesn't look right to mark some 3p lib includes as system ones. Are there any possible side effects from this change?
SiarheiFedartsou commented 2022-12-04 13:19:25 +00:00 (Migrated from github.com)
Review

Well, it seems it depends on compiler.

If the SYSTEM option is given, the compiler will be told the directories are meant as system include directories on some platforms. Signalling this setting might achieve effects such as the compiler skipping warnings, or these fixed-install system files not being considered in dependency calculations - see compiler docs.

https://cmake.org/cmake/help/latest/command/include_directories.html

From my own experience I've never experienced any problems with this myself. I've heard about issue that compiler/cmake may assume those headers never change(i.e. it may break your workflow if you change something in "system" dependency and then iteratively rebuild the project), but not sure this can end up with some problem exactly in this place(after all OMIM_INCLUDE_DIRS contains Boost headers, not sure we will ever use Boost this way)

But in general IMO it is the only way to avoid multiple clang-tidy alerts we won't have any real chance to fix(imagine if someone forgot to add reserve to vector somewhere in Boost header, it is not critical problem, but it will block us from building the project).

Well, it seems it depends on compiler. > If the SYSTEM option is given, the compiler will be told the directories are meant as system include directories on some platforms. Signalling this setting might achieve effects such as the compiler skipping warnings, or these fixed-install system files not being considered in dependency calculations - see compiler docs. https://cmake.org/cmake/help/latest/command/include_directories.html From my own experience I've never experienced any problems with this myself. I've heard about issue that compiler/cmake may assume those headers never change(i.e. it may break your workflow if you change something in "system" dependency and then iteratively rebuild the project), but not sure this can end up with some problem exactly in this place(after all `OMIM_INCLUDE_DIRS ` contains Boost headers, not sure we will ever use Boost this way) But in general IMO it is the only way to avoid multiple clang-tidy alerts we won't have any real chance to fix(imagine if someone forgot to add `reserve` to vector somewhere in Boost header, it is not critical problem, but it will block us from building the project).
biodranik commented 2022-12-04 14:19:34 +00:00 (Migrated from github.com)
Review

Ok. Please double-check about cmake behavior, because we periodically update the boost version and other deps. It would be a pity to waste a lot of time by debugging it later.

Ok. Please double-check about cmake behavior, because we periodically update the boost version and other deps. It would be a pity to waste a lot of time by debugging it later.
target_link_libraries(${executable}
-fsanitize=address
@ -74,12 +74,15 @@ function(omim_add_executable executable)
endif()
endfunction()
# SYSTEM is needed to suppress warnings from 3party headers.
function(omim_add_library library)
add_library(${library} ${ARGN})
# Enable warnings for all our libraries.
target_compile_options(${library} PRIVATE ${OMIM_WARNING_FLAGS})
target_include_directories(${library} PRIVATE ${OMIM_INCLUDE_DIRS})
target_include_directories(${library} SYSTEM PRIVATE ${OMIM_INCLUDE_DIRS})
if (USE_PPROF AND PLATFORM_MAC)
find_path(PPROF_INCLUDE_DIR NAMES gperftools/profiler.h)
target_include_directories(${library} SYSTEM PUBLIC ${PPROF_INCLUDE_DIR})
@ -87,6 +90,18 @@ function(omim_add_library library)
if (USE_PCH)
add_precompiled_headers_to_target(${library} ${OMIM_PCH_TARGET_NAME})
endif()
if (USE_CLANG_TIDY)
find_program(CLANG_TIDY_COMMAND NAMES clang-tidy)
if(NOT CLANG_TIDY_COMMAND)
message(FATAL_ERROR "USE_CLANG_TIDY is ON but clang-tidy is not found!")
else()
message(STATUS "Found clang-tidy at ${CLANG_TIDY_COMMAND}")
set_target_properties(${library} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
endif()
endif()
endfunction()
function(omim_add_test_impl disable_platform_init executable)
@ -96,7 +111,7 @@ function(omim_add_test_impl disable_platform_init executable)
${OMIM_ROOT}/testing/testingmain.cpp
)
target_compile_options(${executable} PRIVATE ${OMIM_WARNING_FLAGS})
target_include_directories(${executable} PRIVATE ${OMIM_INCLUDE_DIRS})
target_include_directories(${executable} SYSTEM PRIVATE ${OMIM_INCLUDE_DIRS})
if(disable_platform_init)
target_compile_definitions(${PROJECT_NAME} PRIVATE OMIM_UNIT_TEST_DISABLE_PLATFORM_INIT)
else()

View file

@ -227,7 +227,7 @@ public:
SubElements(O5MSource * reader, TSubElementGetter const & func) : m_reader(reader), m_func(func) {}
void Skip() { while (m_reader && m_func(nullptr)); }
void Skip() { while (m_reader && m_func(nullptr)) { /* no-op */ } }
Iterator const begin() const { return Iterator(m_reader, m_func); }
Iterator const end() const { return Iterator(); }

View file

@ -52,16 +52,16 @@ public:
{
// String format: <<lat;lon;id;is_capital>>.
// First ';'.
auto pos = oneLine.find(";");
auto pos = oneLine.find(';');
if (pos != std::string::npos)
{
// Second ';'.
pos = oneLine.find(";", pos + 1);
pos = oneLine.find(';', pos + 1);
if (pos != std::string::npos)
{
uint64_t nodeId;
// Third ';'.
auto endPos = oneLine.find(";", pos + 1);
auto endPos = oneLine.find(';', pos + 1);
if (endPos != std::string::npos)
{
if (strings::to_uint64(oneLine.substr(pos + 1, endPos - pos - 1), nodeId))

View file

@ -290,7 +290,7 @@ bool ValidateWebsite(string const & site)
if ('.' == site[startPos] || '.' == site.back())
return false;
if (string::npos == site.find("."))
if (string::npos == site.find('.'))
return false;
if (string::npos != site.find(".."))

View file

@ -807,6 +807,7 @@ void TransitRouteDisplay::CreateTransitMarks()
GetTransitMarkerSizes(kStopMarkerScale, m_maxSubrouteWidth);
vector<m2::PointF> transferArrowOffsets;
transferArrowOffsets.reserve(transferMarkerSizes.size());
for (auto const & size : transferMarkerSizes)
transferArrowOffsets.emplace_back(0.0f, size.y * 0.5f);

View file

@ -12,6 +12,7 @@ namespace openlr
vector<m2::PointD> LinearSegment::GetMercatorPoints() const
{
vector<m2::PointD> points;
points.reserve(m_locationReference.m_points.size());
for (auto const & point : m_locationReference.m_points)
points.push_back(mercator::FromLatLon(point.m_latLon));
return points;

View file

@ -175,7 +175,7 @@ string HttpClient::NormalizeServerCookies(string && cookies)
// Split by ", ". Can have invalid tokens here, expires= can also contain a comma.
while (getline(is, str, ','))
{
size_t const leading = str.find_first_not_of(" ");
size_t const leading = str.find_first_not_of(' ');
if (leading != string::npos)
str.substr(leading).swap(str);
@ -193,7 +193,7 @@ string HttpClient::NormalizeServerCookies(string && cookies)
result.append("; ");
// Read cookie itself.
result.append(str, 0, str.find(";"));
result.append(str, 0, str.find(';'));
}
return result;
}

View file

@ -67,6 +67,7 @@ bool MatchResults(DataSource const & dataSource, vector<shared_ptr<MatchingRule>
vector<search::Result> const & actual)
{
vector<FeatureID> resultIds;
resultIds.reserve(actual.size());
for (auto const & a : actual)
resultIds.push_back(a.GetFeatureID());
sort(resultIds.begin(), resultIds.end());

View file

@ -21,7 +21,7 @@ void CountryInfo::FileName2FullName(string & fName)
// static
void CountryInfo::FullName2GroupAndMap(string const & fName, string & group, string & map)
{
size_t pos = fName.find(",");
size_t pos = fName.find(',');
if (pos == string::npos)
{
map = fName;

View file

@ -182,6 +182,7 @@ TimeTable GetTimeTableFromJson(json_t * obj)
std::vector<TimeInterval> timeIntervals;
auto const & rawValues = GetVectorFromJson<uint64_t>(item, "intervals");
timeIntervals.reserve(rawValues.size());
for (auto const & rawValue : rawValues)
timeIntervals.push_back(TimeInterval(rawValue));
biodranik commented 2022-12-04 12:55:52 +00:00 (Migrated from github.com)
Review

emplace_back?

emplace_back?

View file

@ -134,6 +134,7 @@ template <class Id, class Item>
void UpdateItems(set<Id> const & ids, vector<Item> & items)
{
vector<Item> itemsToFill;
itemsToFill.reserve(ids.size());
for (auto const id : ids)
biodranik commented 2022-12-04 12:56:38 +00:00 (Migrated from github.com)
Review

Is id here a POD type? Or the loop would be faster with a const reference?

Is id here a POD type? Or the loop would be faster with a const reference?
SiarheiFedartsou commented 2022-12-04 13:56:21 +00:00 (Migrated from github.com)
Review

Yeah, at glance it seems that Id can be either StopId or NetworkId which are uint64_t/uint32_t correspondingly.

Yeah, at glance it seems that `Id` can be either `StopId` or `NetworkId` which are uint64_t/uint32_t correspondingly.
itemsToFill.push_back(FindById(items, id));