WIP: Configure clang-tidy CI check #4022

rtsisyk · 2022-12-04T12:20:10Z

SiarheiFedartsou commented

2022-12-04 12:20:10 +00:00

(Migrated from github.com)

Signed-off-by: Siarhei Fedartsou siarhei.fedartsou@gmail.com

Signed-off-by: Siarhei Fedartsou <siarhei.fedartsou@gmail.com>

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 12:22:55 +00:00

.clang-tidy

					
				@ -0,0 +1,114 @@

				---

				Checks: >

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 12:22:54 +00:00

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.

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 12:26:24 +00:00

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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 (Migrated from github.com) commented

2022-12-04 12:26:24 +00:00

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.

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 12:29:29 +00:00

cmake/OmimHelpers.cmake

					
				@ -90,3 +104,4 @@

				endfunction()

				function(omim_add_test_impl disable_platform_init executable)

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 12:29:29 +00:00

I assumed that all targets configured with omim_add_library are considered our "own" targets, but not thirdparty deps, but there are a couple of libraries from 3party folder configured with this omim_add_library too, i.e. clang-tidy checks are applied to them too. As I understand we changed those libraries somehow and that's why consider them as "own"? Or I wrongly get purpose of omim_add_library?

I assumed that all targets configured with `omim_add_library` are considered our "own" targets, but not thirdparty deps, but there are a couple of libraries from `3party` folder configured with this `omim_add_library` too, i.e. clang-tidy checks are applied to them too. As I understand we changed those libraries somehow and that's why consider them as "own"? Or I wrongly get purpose of `omim_add_library`?

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 12:30:22 +00:00

generator/osm_o5m_source.hpp

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 12:30:21 +00:00

Example how false positives can be suppressed

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 12:31:20 +00:00

routing/CMakeLists.txt

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 12:31:20 +00:00

Had to move skarupke lib to nested folder to be able to declare it as "system"(clang-tidy doesn't alert about problems in "system" headers)

Had to move `skarupke` lib to nested folder to be able to declare it as "system"(clang-tidy doesn't alert about problems in "system" headers)

biodranik (Migrated from github.com) requested changes 2022-12-04 13:06:43 +00:00

.clang-tidy

					
				@ -0,0 +1,114 @@

				---

				Checks: >

biodranik (Migrated from github.com) commented

2022-12-04 12:48:05 +00:00

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

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

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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

biodranik (Migrated from github.com) commented

2022-12-04 12:49:57 +00:00

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.

cmake/OmimHelpers.cmake

					
				@ -35,3 +35,3 @@

				  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 (Migrated from github.com) commented

2022-12-04 13:06:18 +00:00

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

cmake/OmimHelpers.cmake

					
				@ -90,3 +104,4 @@

				endfunction()

				function(omim_add_test_impl disable_platform_init executable)

biodranik (Migrated from github.com) commented

2022-12-04 13:05:27 +00:00

Many 3p libs are needed for OM. We add most of them with omim_add_library for convenience. Some libs in 3p, like opening hours, are also our own ones (it should be completely refactored btw and moved into the main source tree).

cmake/OmimHelpers.cmake

biodranik (Migrated from github.com) commented

2022-12-04 13:02:25 +00:00

Does it make sense to avoid adding another option and leverage the CXX_CLANG_TIDY that is set from the command line by -DCXX_CLANG_TIDY=...?

Does it make sense to avoid adding another option and leverage the CXX_CLANG_TIDY that is set from the command line by `-DCXX_CLANG_TIDY=...`?

cmake/OmimHelpers.cmake

biodranik (Migrated from github.com) commented

2022-12-04 13:00:57 +00:00

Why is it needed here? A comment in the code would explain it.

generator/osm_o5m_source.hpp

biodranik (Migrated from github.com) commented

2022-12-04 12:59:55 +00:00

Could the loop below be fixed in a more explicit way? E.g. by adding an empty body?

routing/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-12-04 12:53:48 +00:00

Comments like these should be in the code.
Renaming is not good.
Can it be done without renaming?
What were the issues with skarupke?
Is there any newer version of skarupke with these issues fixed (or with other improvements)?

1. Comments like these should be in the code. 2. Renaming is not good. 3. Can it be done without renaming? 4. What were the issues with skarupke? 5. Is there any newer version of skarupke with these issues fixed (or with other improvements)?

routing/geometry.hpp

biodranik (Migrated from github.com) commented

2022-12-04 12:54:40 +00:00

Just an idea: is it possible to add a wrapper include into std/ dir and disable tidy for the whole include there, in one place?

Just an idea: is it possible to add a wrapper include into `std/` dir and disable tidy for the whole include there, in one place?

transit/experimental/transit_data.cpp

					
				@ -184,3 +184,4 @@

				    auto const & rawValues = GetVectorFromJson<uint64_t>(item, "intervals");

				    timeIntervals.reserve(rawValues.size());

				    for (auto const & rawValue : rawValues)

				      timeIntervals.push_back(TimeInterval(rawValue));

biodranik (Migrated from github.com) commented

2022-12-04 12:55:52 +00:00

emplace_back?

transit/transit_graph_data.cpp

					
				@ -135,3 +135,4 @@

				{

				  vector<Item> itemsToFill;

				  itemsToFill.reserve(ids.size());

				  for (auto const id : ids)

biodranik (Migrated from github.com) commented

2022-12-04 12:56:38 +00:00

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

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 13:11:25 +00:00

cmake/OmimHelpers.cmake

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 13:11:25 +00:00

I don't think so. The problem is that we have to set it to one targets, but don't want to set it to other ones. We probably could do smth like this with CMAKE_CXX_CLANG_TIDY, but in this case clang tidy applies for the whole project, i.e. if in some dependency we have issue(it can be something trivial like I fixed here - e.g. missed reserve, i.e. usually really nice to be fixed, but likely not a big issue if it is not) - we won't be able to build the project. WDYT?

I don't think so. The problem is that we have to set it to one targets, but don't want to set it to other ones. We probably could do smth like this with `CMAKE_CXX_CLANG_TIDY`, but in this case clang tidy applies for the whole project, i.e. if in some dependency we have issue(it can be something trivial like I fixed here - e.g. missed `reserve`, i.e. usually really nice to be fixed, but likely not a big issue if it is not) - we won't be able to build the project. WDYT?

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 13:19:26 +00:00

cmake/OmimHelpers.cmake

					
				@ -35,3 +35,3 @@

				  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)

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 13:19:25 +00:00

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).

SiarheiFedartsou commented

2022-12-04 13:27:56 +00:00

(Migrated from github.com)

I am continuing to work on it in my own fork https://github.com/SiarheiFedartsou/organicmaps/pull/1/commits as it is easier to debug CI this way(does not require explicit maintainers approval) and will return here when will be done with it.

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 13:47:25 +00:00

.clang-tidy

					
				@ -0,0 +1,114 @@

				---

				Checks: >

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 13:47:25 +00:00

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

SiarheiFedartsou (Migrated from github.com) reviewed 2022-12-04 13:56:22 +00:00

transit/transit_graph_data.cpp

					
				@ -135,3 +135,4 @@

				{

				  vector<Item> itemsToFill;

				  itemsToFill.reserve(ids.size());

				  for (auto const id : ids)

SiarheiFedartsou (Migrated from github.com) commented

2022-12-04 13:56:21 +00:00

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.

SiarheiFedartsou commented

2022-12-04 14:01:45 +00:00

(Migrated from github.com)

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.

AFAIR clang-tidy uses clang compilation database to work, so we have to build project first. More than it here we use convenient CMake/clang-tidy integration which automatically runs clang-tidy while building the project. So I think we have to build project anyway. @biodranik WDYT?

> 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. AFAIR clang-tidy uses [clang compilation database](https://clang.llvm.org/docs/JSONCompilationDatabase.html) to work, so we have to build project first. More than it here we use convenient [CMake/clang-tidy integration](https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html#prop_tgt:%3CLANG%3E_CLANG_TIDY) which automatically runs clang-tidy while building the project. So I think we have to build project anyway. @biodranik WDYT?

biodranik commented

2022-12-04 14:03:43 +00:00

(Migrated from github.com)

Is there a way to run checks without building the project? Or is it replaces the building and doesn't produce binaries now in this form?

biodranik (Migrated from github.com) reviewed 2022-12-04 14:17:34 +00:00

cmake/OmimHelpers.cmake

biodranik (Migrated from github.com) commented

2022-12-04 14:17:34 +00:00

Agree.

biodranik (Migrated from github.com) reviewed 2022-12-04 14:19:35 +00:00

cmake/OmimHelpers.cmake

					
				@ -35,3 +35,3 @@

				  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 (Migrated from github.com) commented

2022-12-04 14:19:34 +00:00

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.

biodranik (Migrated from github.com) reviewed 2022-12-04 14:20:19 +00:00

.clang-tidy

					
				@ -0,0 +1,114 @@

				---

				Checks: >

biodranik (Migrated from github.com) commented

2022-12-04 14:20:18 +00:00

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

biodranik commented

2022-12-04 14:21:17 +00:00

(Migrated from github.com)

It would be great to know how exactly clang-tidy works with cmake.

It would be great to know how _exactly_ clang-tidy works with cmake.

biodranik commented

2022-12-04 14:22:27 +00:00

(Migrated from github.com)

am continuing to work on it in my own fork

After merging at least one PR, you won't need maintainers' approvals for checks any more.

> am continuing to work on it in my own fork After merging at least one PR, you won't need maintainers' approvals for checks any more.

Ferenc- (Migrated from github.com) reviewed 2022-12-04 20:37:57 +00:00

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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

Ferenc- (Migrated from github.com) commented

2022-12-04 20:37:57 +00:00

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 (Migrated from github.com) reviewed 2022-12-04 21:07:06 +00:00

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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 (Migrated from github.com) commented

2022-12-04 21:07:06 +00:00

@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 (Migrated from github.com) reviewed 2022-12-04 22:56:58 +00:00

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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

biodranik (Migrated from github.com) commented

2022-12-04 22:56:57 +00:00

@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 (Migrated from github.com) reviewed 2022-12-05 08:31:50 +00:00

.github/workflows/linux-check.yaml

					
				@ -65,3 +65,3 @@

				        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 (Migrated from github.com) commented

2022-12-05 08:31:50 +00:00

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)

SiarheiFedartsou commented

2022-12-10 16:54:21 +00:00

(Migrated from github.com)

Is there a way to run checks without building the project? Or is it replaces the building and doesn't produce binaries now in this form?

As far as I can tell it actually produces binaries, but everything goes through clang-tidy "proxy". At least I see binaries in folder after such clang-tidy builds(and I can run them).

It would be great to know how exactly clang-tidy works with cmake.

Tbh I couldn't find exhaustive information about this in Google, but tried to substitute CXX_CLANG_TIDY with path to my simple script which just outputs all arguments to command line:

#!/bin/sh
echo "clang-tidy $@"

Then I just ran build to see how CMake configures calls to clang-tidy(btw it is actually get called by build system, i.e. make/ninja/...). One of examples:

clang-tidy --extra-arg-before=--driver-mode=g++ /Users/siarheifedartsou/work/organicmaps/routing/routing_quality/api/google/types.cpp -- /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DDEBUG -I/Users/siarheifedartsou/work/organicmaps -I/Users/siarheifedartsou/work/organicmaps/build/3party/gflags/include -I/Users/siarheifedartsou/work/organicmaps/3party/jansson/jansson/src -I/Users/siarheifedartsou/work/organicmaps/3party/jansson/. -isystem /Users/siarheifedartsou/work/organicmaps/3party/boost -g1 -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk -fPIC -fno-omit-frame-pointer -fcolor-diagnostics -ffast-math -Wall -Wextra -Wno-unused-parameter -Wno-deprecated-declarations -std=c++17 -MD -MT routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o -MF routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o.d -o routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o -c /Users/siarheifedartsou/work/organicmaps/routing/routing_quality/api/google/types.cpp

My understanding(may be wrong though): it just takes the whole command line which would be passed to compiler and passes it to clang-tidy(using its --extra-arg-before parameter), clang-tidy builds file and also additionally applies its checks on top of it(i.e. in this case it does everything "on the fly" without use of compilation database).

WDYT?

> Is there a way to run checks without building the project? Or is it replaces the building and doesn't produce binaries now in this form? As far as I can tell it actually produces binaries, but everything goes through clang-tidy "proxy". At least I see binaries in folder after such clang-tidy builds(and I can run them). > It would be great to know how exactly clang-tidy works with cmake. Tbh I couldn't find exhaustive information about this in Google, but tried to substitute `CXX_CLANG_TIDY` with path to my simple script which just outputs all arguments to command line: ``` #!/bin/sh echo "clang-tidy $@" ``` Then I just ran build to see how CMake configures calls to clang-tidy(btw it is actually get called by build system, i.e. make/ninja/...). One of examples: ``` clang-tidy --extra-arg-before=--driver-mode=g++ /Users/siarheifedartsou/work/organicmaps/routing/routing_quality/api/google/types.cpp -- /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DDEBUG -I/Users/siarheifedartsou/work/organicmaps -I/Users/siarheifedartsou/work/organicmaps/build/3party/gflags/include -I/Users/siarheifedartsou/work/organicmaps/3party/jansson/jansson/src -I/Users/siarheifedartsou/work/organicmaps/3party/jansson/. -isystem /Users/siarheifedartsou/work/organicmaps/3party/boost -g1 -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk -fPIC -fno-omit-frame-pointer -fcolor-diagnostics -ffast-math -Wall -Wextra -Wno-unused-parameter -Wno-deprecated-declarations -std=c++17 -MD -MT routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o -MF routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o.d -o routing/routing_quality/api/CMakeFiles/routing_api.dir/google/types.cpp.o -c /Users/siarheifedartsou/work/organicmaps/routing/routing_quality/api/google/types.cpp ``` My understanding(may be wrong though): it just takes the whole command line which would be passed to compiler and passes it to clang-tidy(using its `--extra-arg-before` parameter), clang-tidy builds file and also additionally applies its checks on top of it(i.e. in this case it does everything "on the fly" without use of compilation database). WDYT?

biodranik commented

2022-12-11 09:11:27 +00:00

(Migrated from github.com)

Let's implement it in the fastest/easiest way possible. If it can not be done without building, that's ok.

Did you check some other projects, how do they do it?

Let's implement it in the fastest/easiest way possible. If it can not be done without building, that's ok. Did you check some other projects, how do they do it?

SiarheiFedartsou commented

2022-12-13 15:06:27 +00:00

(Migrated from github.com)

Did you check some other projects, how do they do it?

Well, this implementation is based on what I've done in https://github.com/Project-OSRM/osrm-backend a couple of month ago. Also I used this approach in the past at my previous work projects. Besides this I quickly checked ClickHouse repo, which IMO is good source of various C++ best practices and they use similar approach(but set this CMAKE_CXX_CLANG_TIDY flag per folder, but not per target as we do here - not sure what is the difference):
a4525bb98f/cmake/clang_tidy.cmake (L35)
d1d2f2c1a4/base/CMakeLists.txt (L2)

The main issue here is dealing with false positives. For example often it can be really convenient to just include generated protobuf files to some of existing targets, but if we have clang-tidy enabled for this target it can lead to unwanted clang-tidy warnings. Right way of doing things with such approach is to extract all protobuf-generated files to separate target. There are a lot of examples like this, but IMO clang-tidy gives more benefits than problems in general.

> Did you check some other projects, how do they do it? Well, this implementation is based on what I've done in https://github.com/Project-OSRM/osrm-backend a couple of month ago. Also I used this approach in the past at my previous work projects. Besides this I quickly checked ClickHouse repo, which IMO is good source of various C++ best practices and they use similar approach(but set this `CMAKE_CXX_CLANG_TIDY` flag per folder, but not per target as we do here - not sure what is the difference): https://github.com/ClickHouse/ClickHouse/blob/a4525bb98fbba22d6edac138e4c69dc3d1648e13/cmake/clang_tidy.cmake#L35 https://github.com/ClickHouse/ClickHouse/blob/d1d2f2c1a4979d17b7d58f591f56346bc79278f8/base/CMakeLists.txt#L2 The main issue here is dealing with false positives. For example often it can be really convenient to just include generated protobuf files to some of existing targets, but if we have clang-tidy enabled for this target it can lead to unwanted clang-tidy warnings. Right way of doing things with such approach is to extract all protobuf-generated files to separate target. There are a lot of examples like this, but IMO clang-tidy gives more benefits than problems in general.

biodranik commented

2022-12-13 15:16:11 +00:00

(Migrated from github.com)

It may make sense to check the whole project now, fix the most critical issues, and then decide on the safest integration separately. Spending much time fighting with clang-tidy and its false positives maybe not be the best time investment now. There are other important tasks to do ;-)

SiarheiFedartsou commented

2022-12-13 15:18:08 +00:00

(Migrated from github.com)

It may make sense to check the whole project now, fix the most critical issues, and then decide on the safest integration separately.

Do you mean to create separate PR fixing these(and may be some other) issues? And then return to the question on how to configure it on CI?

> It may make sense to check the whole project now, fix the most critical issues, and then decide on the safest integration separately. Do you mean to create separate PR fixing these(and may be some other) issues? And then return to the question on how to configure it on CI?

biodranik commented

2022-12-13 15:27:17 +00:00

(Migrated from github.com)

Fixes of course can/should be merged separately. No need to wait.

rtsisyk commented

2022-12-17 07:04:57 +00:00

Please separate changes related to adding this new tooling and fixes of bugs in the code. Otherwise, this discussion will never finish. The new tooling can be merged even before all bugs are fixed if it doesn't affect regular workflow and doesn't block PRs. We can always enforce it later after fixing all found bugs (which may take a lot of time).

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik