rsmith added inline comments.
================
Comment at: include/clang/AST/SourceLocExprScope.h:39
+ // static_assert(foo() == __LINE__);
+ return OldVal == nullptr || isa<CXXDefaultInitExpr>(DefaultExpr) ||
+ !OldVal->isInSameContext(EvalContext);
----------------
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
```
================
Comment at: include/clang/AST/SourceLocExprScope.h:44
+public:
+ SourceLocExprScopeBase(const Expr *DefaultExpr, SourceLocExprScopeBase
**Dest,
+ EvalContextType *EvalContext)
----------------
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 **`.
================
Comment at: include/clang/Sema/Sema.h:4463
+
+ /// \brief build a potentially resolved SourceLocExpr.
+ ///
----------------
Capitalize "build" here.
================
Comment at: include/clang/Serialization/ASTBitCodes.h:1643
+ /// A SourceLocExpr record
+ EXPR_SOURCE_LOC,
----------------
Missing period.
================
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;
----------------
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.)
================
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;
----------------
Do we really need both of these? It would seem like one 'current' scope would
be sufficient to me.
================
Comment at: lib/AST/ExprConstant.cpp:7296
+bool IntExprEvaluator::VisitSourceLocExpr(const SourceLocExpr *E) {
+ assert(E && E->isLineOrColumn());
+ llvm::APInt Result;
----------------
Too much indentation.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:192-199
+ void VisitVAArgExpr(VAArgExpr * E);
- void EmitInitializationToLValue(Expr *E, LValue Address);
- void EmitNullInitializationToLValue(LValue Address);
- // case Expr::ChooseExprClass:
- void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
- void VisitAtomicExpr(AtomicExpr *E) {
- RValue Res = CGF.EmitAtomicExpr(E);
- EmitFinalDestCopy(E->getType(), Res);
- }
+ void EmitInitializationToLValue(Expr * E, LValue Address);
+ void EmitNullInitializationToLValue(LValue Address);
+ // case Expr::ChooseExprClass:
+ void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
+ void VisitAtomicExpr(AtomicExpr * E) {
----------------
Something funny happened to the formatting here.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:595
+ } else {
+ StringLiteral *Str = SLE->getStringValue(Ctx, Loc, DC);
+ return CGF.EmitArrayToPointerDecay(Str).getPointer();
----------------
Should we make some attempt to reuse the same string literal for multiple
source locations denoting the same file?
https://reviews.llvm.org/D37035
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
