WIP: Configure clang-tidy CI check #4022
114
.clang-tidy
Normal file
|
@ -0,0 +1,114 @@
|
|||
---
|
||||
Checks: >
|
||||
![]() Let's focus on important checks/fixes that may break the production. Let's focus on _important_ checks/fixes that may break the production.
![]() 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?
![]() 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: '.*'
|
2
.github/workflows/linux-check.yaml
vendored
|
@ -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
|
||||
|
||||
![]() 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:
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.
![]() 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.
![]()
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. BTW, if we are into static checking, then there is this frontend, which next to > 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..
![]() @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)
![]() @Ferenc- did you use codechecker before? Does it integrate easily with CMake? @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.
![]() 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
|
||||
|
|
|
@ -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()
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -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)
|
||||
![]() 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?
![]() Well, it seems it depends on compiler.
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 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 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).
![]() 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()
|
||||
|
|
|
@ -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(); }
|
||||
|
|
|
@ -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))
|
||||
|
|
|
@ -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(".."))
|
||||
|
|
|
@ -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);
|
||||
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -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());
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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));
|
||||
![]() emplace_back? emplace_back?
|
||||
|
||||
|
|
|
@ -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)
|
||||
![]() 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?
![]() Yeah, at glance it seems that 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));
|
||||
|
||||
|
|
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.