[clang-tidy][NFC] Remove unnecessary nullptr check on cast subexpr

[mikerice1969]

The value of SubExpr is not null since getSubExpr would assert in that case. Remove the nullptr check. This avoids confusion since SubExpr is used without check later in the function.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Mike Rice (mikerice1969)

Changes

The value of SubExpr is not null since getSubExpr would assert in that case. Remove the nullptr check. This avoids confusion since SubExpr is used without check later in the function.

---

Full diff: https://github.com/llvm/llvm-project/pull/85473.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp (+1-2)

diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index 4f02950e7794cb..74152c6034510b 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -81,8 +81,7 @@ void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
 
   const Expr *SubExpr = Cast->getSubExpr();
 
-  bool NeedInnerParens =
-      SubExpr != nullptr && utils::fixit::areParensNeededForStatement(*SubExpr);
+  bool NeedInnerParens = utils::fixit::areParensNeededForStatement(*SubExpr);
   bool NeedOuterParens =
       Parent != nullptr && utils::fixit::areParensNeededForStatement(*Parent);
 

[PiotrZSL]

First this null-ptr check does nothing because in line 114 we still missing nullptr check.
Proper way would be to "return" when we find nullptr were it shoudnt.

Second thing, asserts mean nothing. In release mode they are disabled. This mean that if we get nullptr, then we going to crash.

Change is fine for me, but not because there is assert but because in line 114 dereference will happened anyway.

[carlosgalvezp]

Shouldn't we simply return?

As you say, the assert is disabled in Release mode. Even if it's enabled, the author if getSubExpr may one day decide to remove the assert without possibly knowing all the places that do rely on that assert, leaving the code unprotected. It seems like it's trivial solution to return and be on the safe side.

[mikerice1969]

I mentioned the assert just to make the point that setSubExpr is written so it doesn't return a nullptr. So it is not expected and the deference is ok.

I went ahead with this change but feel free to update if you like. But adding a return when nullptr is seen here would confuse the code and static verifiers will complain that the return can never happen.