Enable clang-tidy readability checks #936
Conversation
Baltoli
force-pushed
the
clang-tidy-readability
branch
from
46b8b9b
to
9188f61
Compare
Baltoli
force-pushed
the
clang-tidy-readability
branch
from
8df3e11
to
2d04fe9
Compare
Baltoli
force-pushed
the
clang-tidy-readability
branch
from
cbe0beb
to
cb73c30
Compare
| @@ -40,12 +40,13 @@ void addMatchSuccess(void) { | |||
| nullptr}); | |||
| } | |||
|
|
|||
| void addMatchFailReason(void *subject, char *pattern, char *sort) { | |||
| void addMatchFailReason(void *subject, const char *pattern, const char *sort) { | |||
There was a problem hiding this comment.
Looks like we use both const char * and char const * in various places.
We should consider setting set QualifierAlignment: Right to clean this up: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#qualifieralignment
There was a problem hiding this comment.
Isn't there some kind of semantic difference between the two? The document you linked there even gives a warning about incorrect formatting.
There was a problem hiding this comment.
@gtrepta const char * and char const * are equivalent - both are a pointer to a constant char. I think the common lingo here is "west const" versus "east const".
(Both are distinct from char * const though - a constant pointer to a char.)
The warning is worth bikeshedding about, but IMO it's not a big concern. Looking through clang-format's git blame for the warning, AFAICT the only known issue is that lower-case macros may be mis-identified as types, meaning that it could rewrite
#define intptr int *
const intptr a;
to
intptr const a;
which is a semantic change. This doesn't seem to appear anywhere in our codebase, and if it does, we should just change it to a typedef instead of a macro anyways.
There was a problem hiding this comment.
I think that clang-tidy will actually catch usages of lower-case macros like that anyway, so I think we can make this change safely.
| auto *i1_ty = llvm::Type::getInt1Ty(Ctx); | ||
|
|
||
| auto *proof_output_flag = Module->getOrInsertGlobal("proof_output", i1_ty); | ||
| auto *proof_output = new llvm::LoadInst( | ||
| i1_ty, proof_output_flag, "proof_output", insert_at_end); | ||
| i1_ty, proof_output_flag, "proof_output", insertAtEnd); |
There was a problem hiding this comment.
We should configure readability-identifier-naming to clean up these mismatched naming conventions: https://clang.llvm.org/extra/clang-tidy/checks/readability/identifier-naming.html
There was a problem hiding this comment.
I recall some discussion earlier where lower_case was vouched for, and the reasons sounded compelling. I don't remember the specifics but it does feel easier to read imo than any of the camel case styles.
There was a problem hiding this comment.
Yeah I'm pretty strongly in favour of snake_case naming. I'll put together an issue that makes a choice for all the salient options to readability-identifier-naming so that we can see what it looks like in practice.
|
Yep, I'll fix those comments in follow-ups. The naming convention in particular needs some bikeshedding first, but I plan to get round to it. |
As suggested in #936, this PR sets the appropriate `clang-format` option to right-align qualifiers in the backend (that is, writing `T const&` rather than `const T&`). This PR is fully mechanical and introduces no manual changes to the code. --------- Co-authored-by: rv-jenkins <admin@runtimeverification.com>
This is another PR in my ongoing series to enable more clang-tidy checks for the codebase. As previously, each commit in this PR fixes the issues flagged by a single check, and can / should be reviewed independently.
As ever, I'm happy to bikeshed any of the specific changes in this PR. Note that for
readability-function-cognitive-complexity, I've not actually refactored any existing code in the interest of keeping this diff minimal, but the check will prevent us from introducing new code that exceeds the complexity threshold.