Fixes PR35772.
Details
Diff Detail
- Build Status
Buildable 15037 Build 15037: arc lint + arc unit
Event Timeline
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
10172–10188 | Neither EM_ConstantFold nor EM_IgnoreSideEffects seem like the right evaluation modes to be using to evaluate a non-type template argument. I would expect EM_ConstantExpression to be used for both cases. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
10172–10188 | Oh, I see, we're allowing side-effects here and then issuing a constant-folding warning elsewhere if there actually were any. *sigh* I would expect we can get away with not doing that for template arguments. It'd be worth testing whether GCC actually allows constant folding there, or requires a real constant expression. | |
clang/lib/Sema/SemaOverload.cpp | ||
5401 | Don't we get here for CCEKinds other than the non-type template argument case? |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
10172–10188 | "Allowing side effects" here looks like it really means allowing non-constant subexpressions when those subexpressions would be discarded: template <int *N> struct Foo {}; int x; Foo<(x, &x)> f; constexpr bool is_true = true; int *fn() { if constexpr (((void)x, &x) != nullptr) { return &x; } else { return nullptr; } if constexpr (is_true || x == 2) { return &x; } else { return nullptr; } } Clang accepts this, GCC rejects a few. I'm guessing we want that kind of behavior for if constexpr conditions, but maybe not for non-type template parameters. So, it sounds like we should check the CCE kind in SemaOverload, and use a more restrictive constant expression evaluation mode in that case. Perhaps even a new one for non-type template arguments. | |
clang/lib/Sema/SemaOverload.cpp | ||
5401 | You're right, but I wasn't able to construct a test case where we would call CheckConvertedConstantExpression and we would reject it today because it is dllimported. This was my best idea, using if constexpr: struct __declspec(dllimport) Foo { static constexpr bool imported_foo = true; }; const bool some_bool = false; const bool *f() { if constexpr (Foo::imported_foo) { return &Foo::imported_foo; } else { return &some_bool; } } Any other ideas on how to get more coverage of this path through CheckConvertedConstantExpression? |
clang/include/clang/AST/Expr.h | ||
---|---|---|
103 | If we end up moving this out of Sema into the clang namespace, it needs a longer name than this. | |
670–672 | Seems strange to pass a converted constant expression kind to an 'evaluate as core constant expression' function. And it seems like we don't need the kind here, just an "evaluating for emission w/relocations" vs "evaluating for mangling" enum. Also, we need to evaluate non-type template arguments as constant expressions, not just as core constant expressions, which the implementation does, but the name and comment here don't reflect. (The difference is that you can't result in a pointer/reference to a temporary or automatic / thread storage duration entity.) |
clang/include/clang/AST/Expr.h | ||
---|---|---|
670–672 | So... what are these things? Converted constant expressions? What are we evaluating them as? I guess they're not rvalues or lvalues. |
clang/include/clang/AST/Expr.h | ||
---|---|---|
670–672 | I think this should just be called EvaluateAsConstantExpr, and you should drop all the "core"s throughout. The difference between a constant expression and a core constant expression is that a core constant expression allows the evaluated value to refer to entities with automatic storage duration etc, which is exactly the case you want to disallow here :) I also don't think you need the ParamType, and can instead just look at whether the expression itself is an rvalue. | |
672 | I would prefer an enum { EvaluateForCodeGen, EvaluateForMangling } over a bool IsTemplateArg; I would not be surprised if we find other cases where we want to evaluate an expression for mangling purposes only. | |
clang/lib/AST/ExprConstant.cpp | ||
652 | No "core" here. | |
654 | I don't think we need this. See below. | |
10179–10180 | Instead of a new EvaluationMode which is actually not used by evaluation, we could pass a parameter into CheckLValueConstantExpression to indicate if we're in the "for-mangling" mode. | |
10192 | If you switch from using ParamType->isReferenceType() to asking the expression for its value category, you don't need the implicit-lvalue-to-rvalue-conversion semantics of EvaluateAsRValue, and can just call ::Evaluate here followed by a call to CheckConstantExpression (passing the latter the IsTemplateArg flag). In fact, you don't need the separate case for lvalues above, either, since ::Evaluate does the right thing for an lvalue expression by itself. | |
clang/lib/Sema/SemaOverload.cpp | ||
5401 | All the other forms of CCEKind also set RequireInt to true, and so any case your check catches would also be caught on the line below. |
clang/include/clang/AST/Expr.h | ||
---|---|---|
671 | I expect we could come up with a better name, but is this closer to what you had in mind? |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
1814–1845 | Usage should be passed into these recursive calls to CheckConstantExpression, although that's NFC for now, until/unless we start allowing class types as template parameters. |
If we end up moving this out of Sema into the clang namespace, it needs a longer name than this.