[PATCH] D86176: [clang-tidy] readability-simplify-boolean-expr detects negated literals
Nathan James via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 20 07:02:13 PDT 2020
njames93 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:66
+ if (const auto *Negated = Result.Nodes.getNodeAs<UnaryOperator>(Id)) {
+ if (Negated->getOpcode() == UO_LNot &&
+ isa<CXXBoolLiteralExpr>(Negated->getSubExpr()))
----------------
aaron.ballman wrote:
> I'd like to see this handled recursively instead so that we properly handle more esoteric logic (but it still shows up from time to time) like `!!foo`: https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=%21%21&search=Search
>
> This is a case where I don't think the simplification should happen -- e.g., the code is usually subtly incorrect when converted to remove the `!!`. It's less clear to me whether the same is true for something like `!!!` being converted to `!`, but that's not a case I'm really worried about either.
As this is only looking for boolean literals that shouldn't matter
`!!true` is identical to `true`.
I get putting `!!` in front of expressions that aren't boolean has an effect, but we aren't looking for that in this case.
================
Comment at: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:74
+internal::BindableMatcher<Stmt> literalOrNegatedBool(bool Value) {
+ return expr(anyOf(cxxBoolLiteral(equals(Value)),
+ unaryOperator(hasUnaryOperand(ignoringParenImpCasts(
----------------
aaron.ballman wrote:
> Oof, but handling it here may be tricky...
A custom matcher could be made for this that could traverse a `UnaryOperator`
================
Comment at: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:289
}
+ if (const auto *Unary = dyn_cast<UnaryOperator>(Ret->getRetValue())) {
+ if (Unary->getOpcode() == UO_LNot) {
----------------
aaron.ballman wrote:
> Same here.
This could be easily handled with some recursion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86176/new/
https://reviews.llvm.org/D86176
More information about the cfe-commits
mailing list