Skip to content

Commit

Permalink
[clang-tidy] Fix cppcoreguidelines-narrowing-conversions false positive
Browse files Browse the repository at this point in the history
Fix https://llvm.org/PR49498.
The check notices 2 sides of a conditional operator have types with a different constness and so tries to examine the implicit cast.
As one side is infinity, the float narrowing detection sees when its casted to a double(which it already was) it thinks the result is out of range.

I've fixed this by just disregarding expressions where the builtin type(without quals) match as no conversion would take place.

However this has opened a can of worms. Currenty `float a = std::numeric_limits<double>::infinity();` is marked as narrowing.
Whats more suspicious is `double a = std::numeric_limits<float>::infinity();` is also marked as narrowing.
It could be argued `double inf -> float inf` is narrowing, but `float inf -> double inf` definitely isnt.

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D98416
  • Loading branch information
njames93 authored and carlosgalvezp committed Apr 15, 2023
1 parent 2d7bb01 commit 4530c3b
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,8 @@ void NarrowingConversionsCheck::handleBinaryOperator(const ASTContext &Context,
const BuiltinType *RhsType = getBuiltinType(Rhs);
if (RhsType == nullptr || LhsType == nullptr)
return;
if (LhsType == RhsType)
return;
if (RhsType->getKind() == BuiltinType::Bool && LhsType->isSignedInteger())
return handleBooleanToSignedIntegral(Context, SourceLoc, Lhs, Rhs);
if (RhsType->isInteger() && LhsType->getKind() == BuiltinType::Bool)
Expand Down Expand Up @@ -549,6 +551,8 @@ void NarrowingConversionsCheck::handleImplicitCast(
const Expr &Rhs = *Cast.getSubExpr();
if (Lhs.isInstantiationDependent() || Rhs.isInstantiationDependent())
return;
if (getBuiltinType(Lhs) == getBuiltinType(Rhs))
return;
if (handleConditionalOperator(Context, Lhs, Rhs))
return;
SourceLocation SourceLoc = Lhs.getExprLoc();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,11 @@ void narrow_fp_constants() {
f = __builtin_nan("0"); // double NaN is not narrowing.
}

double false_positive_const_qualified_cast(bool t) {
double b = 1.0;
constexpr double a = __builtin_huge_val();
// PR49498 The constness difference of 'a' and 'b' results in an implicit cast.
return t ? b : a;
}

} // namespace floats

0 comments on commit 4530c3b

Please sign in to comment.