[clang] False Negative: -Wsign-compare not triggered on instantiated templates

[carlosgalvezp]

Example:

#include <cstdint>

template <typename T>
struct Foo
{
    static constexpr T size{123};
};

void f()
{
    std::int32_t foo{123};
    std::uint32_t bar{321U};
    auto x = foo == bar;                        // OK, -Wsign-compare triggered
    auto y = foo == Foo<std::uint32_t>::size;   // NOK, no warnings
}

https://godbolt.org/z/355WMh6hz

[a-tarasyuk]

This seems related to constant evaluation, where the active bits are smaller than the max uint32_t size.

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 10294 to 10306 in b40ff5a

	if (E->isEqualityOp()) {
	unsigned comparisonWidth = S.Context.getIntWidth(T);
	IntRange unsignedRange =
	GetExprRange(S.Context, unsignedOperand, S.isConstantEvaluatedContext(),
	/*Approximate=*/true);
	// We should never be unable to prove that the unsigned operand is
	// non-negative.
	assert(unsignedRange.NonNegative && "unsigned range includes negative?");
	if (unsignedRange.Width < comparisonWidth)
	return;
	}

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 9571 to 9572 in b40ff5a

	if (E->EvaluateAsRValue(result, C, InConstantContext))
	return GetValueRange(C, result.Val, GetExprType(E), MaxWidth);

llvm-project/clang/lib/Sema/SemaChecking.cpp

Lines 9508 to 9519 in b40ff5a

	static IntRange GetValueRange(ASTContext &C, llvm::APSInt &value,
	unsigned MaxWidth) {
	if (value.isSigned() && value.isNegative())
	return IntRange(value.getSignificantBits(), false);
	if (value.getBitWidth() > MaxWidth)
	value = value.trunc(MaxWidth);
	// isNonNegative() just checks the sign bit without considering
	// signedness.
	return IntRange(value.getActiveBits(), true);
	}

@AaronBallman @cor3ntin @shafik I'm unsure whether the checker should always use the maximum size in this case, or should a warning not be triggered?

[shafik]

I think not diagnosing is the right thing to do when we know the number of active bits like we do in this case.

[carlosgalvezp]

But why the inconsistency between having a plain uint32_t and a static constexpr uint32_t variable inside the class? Even removing the template does not give the warning:
https://godbolt.org/z/q8Kb9j3zc

[AaronBallman]

A non-const uint32_t would require dataflow analysis because we can't trust that the value used for initialization is the value at the point of comparison, so we diagnose speculatively in that case. However, when the value is const(expr), we can be assured that the value used for initialization is the same at the point of comparison, so we can use the number of active bits to determine whether there's a potential bug due to conversion or not. e.g., https://godbolt.org/z/e4qo8Mb34

So I believe this diagnostic is working as designed.

[carlosgalvezp]

Ok, thanks! I understand now that the warning is only active when it truly could be a problem, not as a "general principle" about avoiding comparing integers of different signedness (for which perhaps a clang-tidy check would be more suitable).

[ckwastra]

Is the following expected behavior?

void test() {
  {
    auto x = -1;
    auto y = 1u;
    (void)(x < y); // Clang, GCC: warning: [...] [-Wsign-compare]
  }
  {
    constexpr auto x = -1;
    constexpr auto y = 1u;
    (void)(x < y); // GCC: warning: [...] [-Wsign-compare]
    static_assert(x < y);
    // GCC: error: static assertion failed
    //      note: the comparison reduces to '(4294967295 < 1)'
    // Clang: error: static assertion failed due to requirement 'x < y'
    //        note: expression evaluates to '-1 < 1'
  }
}

Clang does not emit a warning when both operands are constant expressions. In this case, the operand x of type int is implicitly converted to unsigned, resulting in the comparison x < y evaluating to false.

[AaronBallman]

Is the following expected behavior?

void test() {
{
auto x = -1;
auto y = 1u;
(void)(x < y); // Clang, GCC: warning: [...] [-Wsign-compare]
}
{
constexpr auto x = -1;
constexpr auto y = 1u;
(void)(x < y); // GCC: warning: [...] [-Wsign-compare]
static_assert(x < y);
// GCC: error: static assertion failed
// note: the comparison reduces to '(4294967295 < 1)'
// Clang: error: static assertion failed due to requirement 'x < y'
// note: expression evaluates to '-1 < 1'
}
}

Clang does not emit a warning when both operands are constant expressions. In this case, the operand x of type int is implicitly converted to unsigned, resulting in the comparison x < y evaluating to false.

Yes, we emit -Wsign-conversion in that case instead because we know for sure the sign will convert thanks to the constexpr: https://godbolt.org/z/9Mexfer4T