void marked 9 inline comments as done.
void added a comment.
In https://reviews.llvm.org/D52854#1255755, @rsmith wrote:
> Essentially, you would add a `ConstExpr` node (or similar), teach
> `IgnoreImplicit` and friends to step over it
I've been trying to do this to no avail. Here's the code I was working with:
//===----------------------------------------------------------------------===//
// Wrapper Expressions.
//===----------------------------------------------------------------------===//
// ConstExpr is a wrapper class indicating that the expression must be
constant.
// This is useful for something like __builtin_constant_p(), which in some
// contexts is required to be constant before generating LLVM IR.
template <typename T>
class ConstExpr : public Expr {
T *E;
public:
ConstExpr(Expr *E) : E(E) {}
T *getSubExpr() { return E; }
const T *getSubExpr() const { return E; }
static bool classof(const Stmt *T) {
return T->getStmtClass() == ConstExprClass;
}
};
I did it this way because otherwise I would have to forward all calls to
individual expression, which is ugly and made me want to cry. The issue is
getting the `ConstExprClass` variable. Those variables are automatically
generated via macro magick, but those macros aren't set up to handle templates.
Do I need to mangle the macros to support templates, or is there another way to
achieve this?
================
Comment at: lib/AST/ExprConstant.cpp:8162
+ return Success(true, E);
+ if (isa<DeclRefExpr>(Arg->IgnoreParenCasts()) &&
+ E->getCanDelayEvaluation())
----------------
rsmith wrote:
> rsmith wrote:
> > Your `canDelayEvaluation` check does not appear to cover several of the
> > cases we'd need to check for here. Eg:
> >
> > ```
> > void f(int n) {
> > enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error
> > because a's value is non-constant
> > ```
> >
> >
> Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct
> check would be whether the reason we found the expression to be non-constant
> was that it tried to read the value of a local variable or function
> parameter. Eg, for:
>
> ```
> void f(int x, int y) {
> bool k = __builtin_constant_p(3 + x < 5 * y);
> ```
>
> ... we should defer the `__builtin_constant_p` until after optimization. (And
> we should never do this if the expression has side-effects.) But given that
> your goal is merely to improve the status quo, not to exactly match GCC, this
> may be sufficient.
>
> You should at least check that the variable is non-volatile, though. (More
> generally, check that `Arg` doesn't have side-effects.)
I had the side effect check in there before. I removed it for some reason. I'll
re-add it.
================
Comment at: lib/Sema/SemaExpr.cpp:5694
+ MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, InitExpr);
----------------
rsmith wrote:
> Why are you marking this? This isn't (necessarily) a context where a constant
> expression is required. (In C, the overall initializer for a global would be,
> but a local-scope compound literal would not.)
>
> Also, this should be in the `Build` function, not in the `ActOn` function.
The name may be misleading. It's looking to see if it should mark whether it
can delay evaluation or not. I'll change its name to be clearer.
================
Comment at: lib/Sema/SemaExpr.cpp:5799
E->setType(Context.VoidTy); // FIXME: just a place holder for now.
+ MarkBuiltinConstantPCannotDelayEvaluation(E);
return E;
----------------
rsmith wrote:
> Likewise here, I don't see the justification for this.
I can't find an equivalent "Build..." for `InitList` expressions. Could you
point me to it?
https://reviews.llvm.org/D52854
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
