LukeZhuang marked 6 inline comments as done.
LukeZhuang added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+ const Expr *ProbArg = TheCall->getArg(2);
+ if (ProbArg->isValueDependent())
+ return ExprError();
----------------
erichkeane wrote:
> LukeZhuang wrote:
> > erichkeane wrote:
> > > Why is value-dependent an error? The expression should be able to be a
> > > template parameter. For example:
> > >
> > >
> > > struct S {
> > > static constexpr float value = 1.1;
> > > };
> > >
> > > template<typename T>
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, T::value);
> > > }
> > >
> > > should work.
> > >
> > > Additionally, in C++20(though not yet implemented in clang it seems):
> > >
> > > template<float F>
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, F);
> > > }
> > >
> > > Additionally, how about an integer type? It would seem that I should be
> > > able ot do:
> > > template<int I>
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, I); // probability is
> > > obviously 0 or 1
> > > }
> > >
> > Hi, this code is based on Richard's suggestion that checking `ProbArg` is
> > not value-dependent before evaluate. I also saw `EvaluateAsFloat` source
> > code used in CGBuiltin.cpp that it firstly assert the expression is not
> > value-dependent as following:
> > ```
> > bool Expr::EvaluateAsFloat(APFloat &Result, const ASTContext &Ctx,
> > SideEffectsKind AllowSideEffects,
> > bool InConstantContext) const {
> > assert(!isValueDependent() &&
> > "Expression evaluator can't be called on a dependent
> > expression.");
> > ```
> > In fact I am not sure is there anything special should be done when is
> > value-dependent. Do you have suggestions about this? Thank you!
> Typically in dependent cases (though this file doesn't deal with the much),
> we just presume the arguments are valid. The values then get checked when
> instantiated. Tests to show this would also be necessary.
Hi, is that mean I just not not need this check, and then add test case as you
suggest to check it does not yield error, is that correct? Thank you!
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1812
+ Context)) ||
+ !Eval.Val.isFloat()) {
+ Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
----------------
erichkeane wrote:
> BTW, this check should be valid (ensuring its a float), since normal
> conversions should happen as a part of the function call.
Do you mean that when customer pass an integer here and make sure this can
handle? I just test this and find `isFloat()` returns true when this argument
is `constexpr int`. Seems it is automatically converted here.
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
