-
-
Notifications
You must be signed in to change notification settings - Fork 7
Enable clang-tidy at target level #1844
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.
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
Conversation
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
src/core/yaml/CMakeLists.txt
Outdated
sourcemeta_library(NAMESPACE sourcemeta PROJECT core NAME yaml | ||
PRIVATE_HEADERS error.h | ||
SOURCES yaml.cc) | ||
sourcemeta_clang_tidy_attempt_enable(TARGET_NAME "sourcemeta_core_yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you instead do this by default on the definition of sourcemeta_library
and sourcemeta_executable
? Then you don't have to add it for every library
cmake/common/clang-tidy.cmake
Outdated
|
||
function(sourcemeta_clang_tidy_attempt_disable) | ||
unset(CMAKE_CXX_CLANG_TIDY PARENT_SCOPE) | ||
set_target_properties("${SOURCEMETA_CLANG_TIDY_TARGET_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you confirm this works? I haven't tried it yet, but when I tried your past PR, it wouldn't seem to like it called ClangTidy at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tested locally
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I just added a small commit cleaning some stuff up so we can merge it fast.
Ah, I think there is a conflict on the removal of the C++ standard. I'll let you fix that and then we can merge |
@jviotti The conflict is resolved. Can we merge this? |
c606fa8
to
176e98d
Compare
@jviotti Let's merge this before others to avoid merge conflicts on this PR. |
Follow up #1824 (comment)