-
Notifications
You must be signed in to change notification settings - Fork 11
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
chore: fix issues raised by clang-tidy (part 2) #84
Conversation
This Pr fixes the following code issues raised by `cert-*,readability-*,` - simplify nullptr checks - explicit boolean conversions for length() == 0 - always have parentheses on if/for blocks - avoid else block after return/break - use !vector.empty() instead of size() > 0 - use !str.empty() instead of != "" - const member functions - fix inconsistent parameter names between header/impl files - verbose auto for `auto *` and `const auto *` for semantic hints - mark unused parameters with /*unused*/ - some controversial/noisy readability checks are disabled.
@lukhnos please kindly take a look and answer the above questions. Thanks! |
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.
Thank you so much!
I've answered most of your questions in my review comments. As for:
Some "readability rules" are somewhat opinionated, please let me know if you'd like to revert any of them, I can amend the PR.
They are fine, and I think having those in place is in general a good thing for code review purposes.
all warnings from clang-tidy are gone/disabled with this PR, please let me know if you would like to enable clang-tidy in CMake by default.
Just wanted to confirm: it will not attempt to run clang-tidy if it's not available on a local machine, right? If that's the case, I think enabling it by default is a good thing.
@@ -48,6 +48,7 @@ constexpr double kNoOverrideThreshold = -8.0; | |||
|
|||
static const char* GetKeyboardLayoutName( | |||
const Formosa::Mandarin::BopomofoKeyboardLayout* layout) { | |||
// NOLINTBEGIN(readability-else-after-return) |
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.
Hmm this one is a bit surprising. What if you move the last return "Standard";
to a final else branch? Would both gcc and clang be happy with it?
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.
@lukhnos, I didn't find the rule applicable here. It would prefer refactoring the following:
if () {
return a;
} else if () {
return b;
} else if () {
return c;
}
...
into
if () {
return a;
}
if () {
return b;
}
if () {
return c;
}
...
Theoretically, they are identical, but I find this is a bit too lengthy and verbose. We are clearly returning different things w/ given layout.
I believe readability is all about letting the code reader spending minimum mental effort to understand the code intention, which we were clear. My decision is to suppress this rule locally, how do you think?
https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html
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.
I believe readability is all about letting the code reader spending minimum mental effort to understand the code intention, which we were clear. My decision is to suppress this rule locally, how do you think?
SGTM. I agree with your assessment about this particular rule.
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.
Overall LGTM, just that I left a few comments which actually requested some changes. Thanks!
Thank you so much! I'll be a bit busy this weekend, will try to make some time to update this PR. |
If clang-tidy is not found on the mcahine, Cmake should output the following: ``` -- Checking clang-tidy... -- clang-tidy not found, disable clang-tidy checks ```
hi @lukhnos, I enabled |
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.
I've tried your branch. Now that clang-tidy is enabled by default, I found it's reporting a few more warnings in Mandarin.cpp, UTF8Helper.cpp, and KeyHandler.cpp. I'm using clang-tidy 11.0.1.
Do you plan to continue working on the warnings, or should we hold off enable-clang-tidy-by-default until all warnings are fixed? I don't want to put the burdens on you—other maintainers can also work on this.
Otherwise this LGTM!
Edit: since you mentioned earlier that all clang-tidy warnings were fixed with this PR, I wonder if I was using a clang-tidy version that was different from yours, hence the question about whether we should revert to your original setup, which is only to enable clang-tidy if |
@lukhnos interesting ... clang 11.1.0 is old [1], I'm on 15.0.7 (archlinux). Arch actually has 16.0.6 released 2 days ago [2], but I haven't updated just yet. Let me revert my change, I would avoid causing confusion to other maintainers with older versions of clang.
https://github.com/llvm/llvm-project/releases/tag/llvmorg-11.1.0 |
This reverts commit 55e5e2a. Revert this change to avoid confusion for people with older version of clang on other distros.
As an experiment, I added a CI step to a branch in my forked repo. clang version installed: https://github.com/xatier/fcitx5-mcbopomofo/actions/runs/5916113158/job/16043026110#step:4:84
enable clang-tidy: https://github.com/xatier/fcitx5-mcbopomofo/actions/runs/5916113158/job/16043026110#step:4:312
As you can see, there's no warning reported by clang-tidy 15. (and surely, it takes slightly longer to finish) CI log link: https://github.com/xatier/fcitx5-mcbopomofo/actions/runs/5916113158/job/16043026110 |
I think we can always enable it by default once enough of us are on the lastest toolchain. Thank you so much for your contribution! |
Sweet, thanks! Sure, we can always enable it by default later. |
I checked a little deeper with older versions of clang-tidy, it appears that I saw those warnings on Debian stable with clang-tidy 13, but not after clang-tidy 14. We should be good to enable this by default after most distros are shipping with clang 14+. |
This PR fixes the following code issues raised by
cert-*,readability-*,
simplify nullptr checks
explicit boolean conversions for length() == 0
always have parentheses on if/for blocks
avoid else block after return/break
use !vector.empty() instead of size() > 0
use !str.empty() instead of != ""
const member functions
fix inconsistent parameter names between header/impl files
verbose auto for
auto *
andconst auto *
for semantic hintsmark unused parameters with /unused/
some controversial/noisy readability checks are disabled.
@lukhnos, I do have a few remarks/questions:
if (false)
blocks insrc/Engine/Mandarin/Mandarin.cpp
, which I wasn't sure the intention, I left them untouched and suppressed the linter.src/Engine/ParselessPhraseDB.cpp
, can you please share more contexts on these?clang-tidy
are gone/disabled with this PR, please let me know if you would like to enableclang-tidy
in CMake by default.