[clang] Do not diagnose float-equal for 0 and 1

[juniorrantila]

The values 0 and 1 are commonly used for checking if something is initialized and are both exactly representable as floats. Emitting a diagnostic when comparing with these values seems a bit obtuse.

[github-actions]

Hello @juniorrantila 👋

Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.

• All contributions to LLVM must follow our LLVM AI Tool Use Policy. In particular, if you used AI while working on this PR, remember to add a note to the PR description.
• The LLVM Code-Review Policy and Practices document contains practical information about the PR process, including how patches are reviewed and accepted, and who can review a PR.
• Our LLVM Developer Policy describes our expectations for code quality, commit summaries and contains notes on our CI system.

Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description.

---

Frequently asked questions

How do I add reviewers?

This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically.

You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using @ followed by their GitHub username.

What if there are no comments?

If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers.

Are any special GitHub settings required to contribute to LLVM?

We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details.

---

If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse.

Thank you,
The LLVM Community

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

Author: Junior Rantila (juniorrantila)

Changes

The values 0 and 1 are commonly used for checking if something is initialized and are both exactly representable as floats. Emitting a diagnostic when comparing with these values seems a bit obtuse.

---

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

1 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+8)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 01b1f4c26f017..8050d387b0fa3 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11672,6 +11672,14 @@ void Sema::CheckFloatComparison(SourceLocation Loc, const Expr *LHS,
   } else if (const auto *FLR = dyn_cast<FloatingLiteral>(RightExprSansParen))
     if (FLR->isExact())
       return;
+  if (const auto *ILL = dyn_cast<IntegerLiteral>(LeftExprSansParen)) {
+    if (ILL->getValue() == 0 || ILL->getValue() == 1)
+      return;
+  }
+  if (const auto *ILR = dyn_cast<IntegerLiteral>(RightExprSansParen)) {
+    if (ILR->getValue() == 0 || ILR->getValue() == 1)
+      return;
+  }
 
   // Check for comparisons with builtin types.
   if (const auto *CL = dyn_cast<CallExpr>(LeftExprSansParen);

[Sirraide]

Candidly, I don’t think this is a good idea.

The values 0 and 1 are commonly used for checking if something is initialized and are both exactly representable as floats. Emitting a diagnostic when comparing with these values seems a bit obtuse.

Assuming you’re talking about -Wfloat-equal here, I don’t quite agree w/ this notion: the reason we diagnose this is that computations involving floating-point numbers are fundamentally inexact.

A computation that mathematically would result in 0 or 1 may not actually give you 0 or 1 if you’re using floating-point numbers (e.g. .7 + .2 + .1 == 1 evaluates to false because the LHS is actually .9999999999999999), which means that most of the time, == or != are definitely not what you want when you’re working with floating-point numbers.

Whether or not such a comparison happens to involve 1 or 0 doesn’t really make a difference here—if anything not diagnosing a comparison with 0 would be especially bad given that comparisons against 0 are actually quite common (e.g. two vectors are perpendicular if their dot product is 0).

(As an aside, from an implementation perspective, this would still need a test and a release note.)

[Sirraide]

I see we apparently don’t diagnose floating-point literals for some reason... but I don’t think that’s really a good idea either

[juniorrantila]

I am currently on holidays, so will get back to you in a week unless I get a chance to use the computer :p

This patch was inspired by one of the safety critical C guidelines, I don't remember which one at the top of my head, will look it up as soon as I get the opportunity, where floating point equality comparison is not allowed. However, 0 and 1 are special cased to be allowed. Reasoning behind this, as I understand it, is that it is common to memset buffers to 0 as to signify not yet initialised for lazy structures. When you compare to 0, you actually are interested in the exact bit pattern to signify that you need to do initialisation of the structure, approximate comparison would be incorrect. It could be argued that the same idea goes for the value 1 or any other exactly representable float value as well, but I haven't personally needed anything other than 0 or 1.

I will look up the specific section where the exceptions are mentioned as soon as I have time :)

[AaronBallman]

This patch was inspired by one of the safety critical C guidelines, I don't remember which one at the top of my head, will look it up as soon as I get the opportunity, where floating point equality comparison is not allowed. However, 0 and 1 are special cased to be allowed.

That seems surprisingly dangerous given this behavior in the wild: https://godbolt.org/z/dhMxM91n3 (I'm surprised Clang doesn't diagnose that!)

When you compare to 0, you actually are interested in the exact bit pattern

Then you shouldn't be comparing with zero, but instead using memcmp() or inspecting the value representation (through bit casts, union puns, etc).

I'm not opposed, just some drive-by comments. I added a few floating-point experts for their opinions.

[Sirraide]

(I'm surprised Clang doesn't diagnose that!)

Yeah, we explicitly don’t diagnose floating-point literals that are exactly representable:

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

Lines 11664 to 11674 in fb1de34

	// Special case: check for comparisons against literals that can be exactly
	// represented by APFloat. In such cases, do not emit a warning. This
	// is a heuristic: often comparison against such literals are used to
	// detect if a value in a variable has not changed. This clearly can
	// lead to false negatives.
	if (const auto *FLL = dyn_cast<FloatingLiteral>(LeftExprSansParen)) {
	if (FLL->isExact())
	return;
	} else if (const auto *FLR = dyn_cast<FloatingLiteral>(RightExprSansParen))
	if (FLR->isExact())
	return;

I don’t think doing this is a good idea but it’s been there since 2007.

[andykaylor]

The logic that leads us to suppress the warning for exact floating-point literals could easily be extended to integer literals that can be exactly represented, but I'm also not sure it's a good idea to have ever done that. The reasoning for the warning seems to apply to the exact-literal comparison cases as well as anything else. If there really is demand for not diagnosing in these cases, perhaps a new option to control that behavior specifically is in order.

[juniorrantila]

I did some more digging and can't find the exact PDF I was looking for, but in MISRA 1998, rule 40 states:

Floating-point variables shall not be tested for exact equality or inequality.

**How the rule is checked**

The compiler will generate an error, indicating a violation of this rule, if == or != is
applied to a floating-point value. If a comparison is explicitly against the floating-point
constant 0.0, no error message is given.

In MISRA 2004 (Rule 13.3), they removed the comparison to zero exemption.
However, in MISRA 2012 and onward they removed the floating point comparison rule altogether, no longer explicitly disallowing floating point equality comparison, instead it is indirectly disallowed by other rules. MISRA C 2012 Rule 10.1 allows comparisons to 0, INFINITY and -INFINITY. They clarify this in MISRA C:2012 Amendment 3 (2.3.7 Amend Rule 10.1), they allow this because there is code that needs to guard against those specific values, for instance to catch division by zero.

Static analysis tools seem to commonly allow comparisons to 0 and INFINITY and comparisons where it can prove that the value does not drift, for instance:

for (float v = 0.0f; v != 100.0f, v += 1.0f)
  ;

would be allowed on the grounds that the initializer and the value we increment by are both integer values, so the float will not drift. See Polyspace.

In my opinion, 0 and INFINITY should be allowed by default with this -Wfloat-equal, since you commonly want to guard against those values. Additionally, we could have a flag that restricts the use of 0. As for other float values (other than 0 and INFINITY), maybe we should not allow them by default and provide a flag to allow exact float values, this way, people who intend to compare to the same bit pattern can still do so.

[AaronBallman]

In my opinion, 0 and INFINITY should be allowed by default with this -Wfloat-equal, since you commonly want to guard against those values. Additionally, we could have a flag that restricts the use of 0. As for other float values (other than 0 and INFINITY), maybe we should not allow them by default and provide a flag to allow exact float values, this way, people who intend to compare to the same bit pattern can still do so.

I think it's useful to find all direct comparisons of floating-point types regardless of value, which is the behavior of -Wfloat-equal in GCC. It's not that the comparison itself is invalid, it's that it's pointing out a potential algorithmic issue with the computations leading up to the comparison. e.g., https://godbolt.org/z/d69drsM95 But given that this has been Clang's behavior with this diagnostic for almost 20 years, I expect changing Clang to match GCC would be too disruptive. That suggests we need a new warning group, but naming the group will be a challenge.