EricWF marked 10 inline comments as done.
EricWF added inline comments.
================
Comment at: include/clang/AST/SourceLocExprScope.h:39
+ // static_assert(foo() == __LINE__);
+ return OldVal == nullptr || isa<CXXDefaultInitExpr>(DefaultExpr) ||
+ !OldVal->isInSameContext(EvalContext);
----------------
rsmith wrote:
> Do we want the `isa` check here? I'd think the same problem would occur for
> default initializers:
>
> ```
> struct A {
> int n = __builtin_LINE();
> };
> struct B {
> A a = {};
> };
> B b = {}; // should give this context as the current one, not the context of
> the initializer inside struct B
> ```
Good catch. Fixed.
================
Comment at: include/clang/AST/SourceLocExprScope.h:44
+public:
+ SourceLocExprScopeBase(const Expr *DefaultExpr, SourceLocExprScopeBase
**Dest,
+ EvalContextType *EvalContext)
----------------
rsmith wrote:
> I think it'd be cleaner to add a
>
> ```
> struct Current {
> SourceLocExprScopeBase *Scope;
> };
> ```
>
> maybe with members to get the location and context for a given
> `SourceLocExpr`, and to pass in a `SourceLocExprScopeBase::Current &` here
> rather than a `SourceLocExprScopeBase **`.
Done. Hopefully I understood the comment.
================
Comment at: include/clang/Sema/Sema.h:4463
+
+ /// \brief build a potentially resolved SourceLocExpr.
+ ///
----------------
rsmith wrote:
> Capitalize "build" here.
Done and removed `\brief`.
================
Comment at: lib/AST/Expr.cpp:1924-1931
+ auto CreateString = [&](StringRef SVal) {
+ QualType StrTy = BuildSourceLocExprType(Ctx, getIdentType(),
+ /*IsArray*/ true, SVal.size() + 1);
+ StringLiteral *Lit = StringLiteral::Create(Ctx, SVal, StringLiteral::Ascii,
+ /*Pascal*/ false, StrTy, Loc);
+ assert(Lit && "should not be null");
+ return Lit;
----------------
rsmith wrote:
> It doesn't seem reasonable to create a new `StringLiteral` each time a
> `SourceLocExpr` is (constant-)evaluated, and seems like it would be unsound
> in some cases (we require that reevaluation produces the same result).
>
> I think the `StringLiteral` here is just for convenience, right? (Instead we
> could model the `SourceLocExpr` as being its own kind of array lvalue for the
> purpose of constant expression evaluation, like we do for `ObjCEncodeExpr`
> for instance.)
OK. I think I have a solution for this. Please forgive my poor explanation of
the problem. The problem was two fold:
First, the type of `SourceLocExpr` represents the decayed type of the
underlying array, and not the array type or value-category itself (as
`StringLiteral` and `ObjCEncodeExpr` do). Therefore it takes additional work to
propagate and ensure that the existing machinery uses the "real array type" of
the `SourceLocExpr` and not the type of the `SourceLocExpr` itself.
Second,,we need to determine, store, and propagate the type,, value category,
and value of a `SourceLocExpr` corresponding to the context in which we first
encounter the `SourceLocExpr. None of this contextual information is available
in the AST after the fact. A lazy model where `SourceLocExpr` pretends to be a
placeholder for the "real array type and value" until said subelements are
actually accessed will produce different results than if the `SourceLocExpr`
was correctly evaluated in the context it was initially encountered.
For Example:
```
const char *cur_file(const char *f = __builtin_FILE()) { return f; }
struct TestInit {
#line 100 "initial_scope.cpp"
const char *my_file = cur_file();
TestInit() {} // FILE should point here. We need to transform/determine the
value and type of the `SourceLocExpr` in this context.
};
#line 200 "new_scope.cpp"
// If we delay evaluating `__builtin_FILE()` until this point, where it's
actually needed, it will return `new_scope.cpp`,
// because we've already exited the `CXXDefaultInitExpr` scope created when
first evaluating `TestInit() = default`.
TestInit Default;
static_assert(strcmp(Default.my_file, "initial_scope.cpp") == 0); // Fails if
lazily evaluated!
```
I worked around this problem by storing the "real array type", the used
location, and the used context of the `__builtin_FOO())` expression in the
`LValueBase` produced when we first consider the `__builtin_FOO()` expression.
================
Comment at: lib/AST/ExprConstant.cpp:698-704
+ /// \brief Source location information about the default argument
expression
+ /// we're evaluating, if any.
+ SourceLocExprScope *CurCXXDefaultArgScope = nullptr;
+
+ /// \brief Source location information about the default member initializer
+ /// we're evaluating, if any.
+ SourceLocExprScope *CurCXXDefaultInitScope = nullptr;
----------------
rsmith wrote:
> Do we really need both of these? It would seem like one 'current' scope would
> be sufficient to me.
Indeed, we don't seem to need both so I've removed one.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:595
+ } else {
+ StringLiteral *Str = SLE->getStringValue(Ctx, Loc, DC);
+ return CGF.EmitArrayToPointerDecay(Str).getPointer();
----------------
rsmith wrote:
> Should we make some attempt to reuse the same string literal for multiple
> source locations denoting the same file?
I think I've addressed this. Instead of creating a `StringLiteral` expressions
to evaluate, the new code uses `GetAddrOfConstentCString` to create the
constant address corresponding to the string value specified by the
`SourceLocExpr`.
https://reviews.llvm.org/D37035
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
