erichkeane added a comment. Additionally, it seems you're missing SEMA tests. Please add those. Those tests should also make sure that the diagnostics still work with dependent values.
================ Comment at: clang/include/clang/Basic/Builtins.def:567 BUILTIN(__builtin_expect, "LiLiLi" , "nc") +BUILTIN(__builtin_expect_with_probability, "LiLiLid" , "nc") BUILTIN(__builtin_prefetch, "vvC*.", "nc") ---------------- The spaces after the first parameter are odd/unnecessary. I suspect it is in the line above because it was used to do alignment in the past. Please don't bother putting them in this new line. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2197 + const Expr *ProbArg = E->getArg(2); + bool EvalSucceed = ProbArg->EvaluateAsFloat(Probability, + CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects); ---------------- This is going to give an unused variable warning in release mode, since assert is a macro. You'll have to cast EvalSucceeded to void after the assert. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2199 + CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects); + assert(EvalSucceed == true); + llvm::Type *Ty = ConvertType(ProbArg->getType()); ---------------- First, don't do ==true here. Second, add a string to the assert (see other assert uses). 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
