ci: Add clang-tidy to CI #95
Conversation
closes #76
juan-lunarg
requested review from
spencer-lunarg,
charles-lunarg and
christophe-lunarg
| } | ||
|
|
||
| std::string FormatString(const char *message, ...) { | ||
| std::size_t const STRING_BUFFER(4096); | ||
| std::size_t const string_buffer(4096); |
There was a problem hiding this comment.
string_buffer is a confusing name since its the length of the buffer, not the buffer itself.
There was a problem hiding this comment.
I am personnally not in favour of such changes. C.I. making more checks that on local system by default is hunting productivity. And if to increase productivity we need to use the flag, then the flag should be on my default.
Do we have in order already the -WERROR things on repo? Meaning that if C.I. is using it, we should use in by default on our system.
This case is already pretty painful to me, I don't want to deal with another case like this that in this end provide little value expect avoiding pedentic disucssions in code reviews.
Personnally I think that if we add such things, this should be discuss in engineering meetings with the team.
|
|
||
| ### clang-tidy | ||
|
|
||
| By default clang-tidy checking is disabled by default. |
There was a problem hiding this comment.
Why is it disabled by default? It seems much better for me to hit issues on my system than on C.I., consume more C.I. resource and have to iterate, taking more development time. This is especially true that I don't see how the developer would even now what the rules are.
I would argu the same with VUL_WERROR being OFF by default.
I think we need to be more consious when we add new checks how this is going to affect the developers productivity.
There was a problem hiding this comment.
It's off by default to avoid issues with package managers, machines that don't have clang-tidy installed by default, FetchContent/add_subdirectory users, etc.
Having it on by default is more complicated than having it on. That's just the way it is with open source projects.
| @@ -150,17 +150,17 @@ std::filesystem::path LayerSettings::FindSettingsFile() { | |||
| LSTATUS err = RegOpenKeyEx(hives[hive_index], TEXT("Software\\Khronos\\Vulkan\\Settings"), 0, KEY_READ, &key); | |||
| if (err == ERROR_SUCCESS) { | |||
| TCHAR name[2048]; | |||
| DWORD i = 0, name_size, type, pValues, value_size; | |||
| DWORD i = 0, name_size, type, p_values, value_size; | |||
There was a problem hiding this comment.
I have never seen this convention before.
There was a problem hiding this comment.
I grabbed the clang-tidy file from the vulkan validation layers.
There was a problem hiding this comment.
I have no preference tbh. Just a matter of changing the clang-tidy file.
- { key: readability-identifier-naming.VariableCase, value: lower_case }
Another reason to also have it disabled by default is because if your IDE registers .clang-tidy files. (Visual Studio Code, Visual Studio, and many other IDEs do). Is that it will pointlessly eat up build time. Since you can just fix the clang-tidy issues in your IDE. I've looked at various other repos that have implemented clang-tidy and this seems to be the convention from what I can gather.
Understood. This has been brought up multiple times in VVL meetings. It's a lot harder to implement on an existing codebase however. Since it would require a lot of fixes. The thought process was to get it working on a newer repo to iron the kinks in the workflow.
As you said. The main intent behind clang-tidy is to enforce a style guide essentially. It's very flexible/powerful. However, it does more than just style checking. It provides performance static analysis checks, security checks, etc. |
|
To be clear. I do understand the annoyance of adding more checks. But overall I thinks it's better for the health of the multiple code bases we have to manage. |
|
I've got a better idea that would go more smoothly. Closing this PR. |
| if(NOT CLANG_TIDY) | ||
| message(FATAL_ERROR "clang-tidy not found!") | ||
| endif() | ||
| set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY}") |
There was a problem hiding this comment.
CMAKE_<LANG>_CLANG_TIDY only works on the makefile / ninja generator.
Given how popular the visual studio generator is within our team this is likely only going to cause confusion.
closes #76