[PATCH] D79830: Add support of __builtin_expect_with_probability

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819
+    llvm::APFloat Probability = Eval.Val.getFloat();
+    if (!(Probability >= llvm::APFloat(0.0) ||
+          Probability <= llvm::APFloat(1.0))) {
+      Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)
----------------
This doesn't look right to me, but maybe i'm misreading something?
Assuming the `!` is intentional to avoid some fp weirdness (if not, simplify this)
shouldn't this be an `&&`, not `||`?

Consider:
`Probability = -1`
```
    if (!(-1 >= llvm::APFloat(0.0) ||
          -1 <= llvm::APFloat(1.0))) {
```
```
    if (!(false ||
          true) {
```
```
    if (false) {
```
so i'd think `Probability = -1` would not get diagnosed?


================
Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:19-20
+  dummy = __builtin_expect_with_probability(x > 0, 1, 0.9);
+  dummy = __builtin_expect_with_probability(x > 0, 1, 1.1); // expected-error {{probability of __builtin_expect_with_probability is outside the range [0.0, 1.0]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, -1); // expected-error {{probability of __builtin_expect_with_probability is outside the range [0.0, 1.0]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, p); // expected-error {{probability of __builtin_expect_with_probability must be constant floating-point expression}} expected-note {{read of non-constexpr variable 'p' is not allowed in a constant expression}}
----------------
Do these tests actually pass?


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

https://reviews.llvm.org/D79830