[PATCH] D81272: [clang-tidy] New check `misc-redundant-condition`

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I feel the refactoring of Aliasing should be in its own PR, with this being a child of it.



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:38
+      ifStmt(hasCondition(anyOf(
+                 declRefExpr(hasDeclaration(varDecl().bind("cond_var"))),
+                 binaryOperator(hasOperatorName("&&"),
----------------
Small nit, but using string literals for bound nodes is error prone. may I suggest defining some static strings and using those to prevent any typos.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:39
+                 declRefExpr(hasDeclaration(varDecl().bind("cond_var"))),
+                 binaryOperator(hasOperatorName("&&"),
+                                hasEitherOperand(declRefExpr(hasDeclaration(
----------------
Recursive matchers are a pain, but this will miss:
 ```
if (A && B && ... Y && Z)
```



================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:63
+  // Non-local variables may be changed by any call.
+  if (!CondVar->isLocalVarDeclOrParm())
+    return;
----------------
This logic can be moved to the matcher.
```
varDecl(anyOf(parmVarDecl(), hasLocalStorage()))
```


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:67
+  // Volatile variables may be changed by another thread.
+  if (CondVar->getType().isVolatileQualified())
+    return;
----------------
Ditto:
```
varDecl(hasType(isVolatileQualified()))```


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:71
+  // Class instances may be tricky, so restrict ourselves to integers.
+  if (!CondVar->getType().getTypePtr()->isIntegerType())
+    return;
----------------
Ditto, you get the point


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:87-88
+  if (isa<DeclRefExpr>(InnerIf->getCond()->IgnoreParenImpCasts()) ||
+      (isa<BinaryOperator>(InnerIf->getCond()) &&
+       cast<BinaryOperator>(InnerIf->getCond())->getOpcode() == BO_LOr)) {
+    SourceLocation IfBegin = InnerIf->getBeginLoc();
----------------
use `llvm::dyn_cast`.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:97
+                  CharSourceRange::getCharRange(IfBegin, IfEnd))
+           << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+                  Body->getEndLoc(), Body->getEndLoc().getLocWithOffset(1)));
----------------
Again `CharSourceRange::getTokenRange(Body->getEndLoc())`


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:109
+    const auto *CondOp = cast<BinaryOperator>(InnerIf->getCond());
+    if (isa<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts()) &&
+        cast<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts())->getDecl() ==
----------------
use `llvm::dyn_cast`.


================
Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:115
+    } else {
+      // For some reason the end location of identifiers is identical to their
+      // begin location so add their length instead.
----------------
That's because you are getting the character range, if you get the token range it'll get the correct range: `CharSourceRange::getTokenRange`


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:135
    `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
+   `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes"
----------------
baloghadamsoftware wrote:
> What are these changes? I surely did not make them? Maybe the check adder Python script?
Yeah, just undo those unrelated changes, maybe one day the python script will get sorted


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81272/new/

https://reviews.llvm.org/D81272