LukeZhuang marked 4 inline comments as done.
LukeZhuang 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)
----------------
lebedev.ri wrote:
> 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?
Yes, you're right.. Because of version problem, I still use converting to
double in my local build, and miscopy this when upstreaming. Thank you for
point it out.
================
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}}
----------------
lebedev.ri wrote:
> Do these tests actually pass?
It pass it for now. Again my local build still use converting to double to
compare and passed the test, but I miscopied it when upstream the change. Thank
you!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79830/new/
https://reviews.llvm.org/D79830
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
