rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.
Some cleanups and simplifications, but LGTM with those applied. Thank you!
================
Comment at: docs/LanguageExtensions.rst:2335
+
+When the builtins appears as part of a default function argument the invocation
+point is the location of the caller. When the builtins appear as part of a
----------------
appears -> appear
================
Comment at: include/clang/AST/CurrentSourceLocExprScope.h:31
+public:
+ /// A RAII style scope gaurd used for tracking the current source
+ /// location and context as used by the source location builtins
----------------
gaurd -> guard
================
Comment at: include/clang/AST/Expr.h:4168
+/// Represents a function call to one of __builtin_LINE(), __builtin_COLUMN(),
+/// __builtin_FUNCTION(), or __builtin_FILE()
+class SourceLocExpr final : public Expr {
----------------
Missing period at end of comment.
================
Comment at: lib/AST/ASTContext.cpp:10151-10153
+ // Get an array type for the string, according to C99 6.4.5. This includes
+ // the nul terminator character as well as the string length for pascal
+ // strings.
----------------
"the string length for pascal strings" part here seems out of place, since you
don't handle that here.
================
Comment at: lib/AST/Expr.cpp:2073
+ StringLiteral *Res = Ctx.getPredefinedStringLiteralFromCache(Tmp);
+ return APValue(Res, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+ };
----------------
Given that the type of the `SourceLocExpr` is `const char*`, I'd expect the
`APValue` result to be a decayed pointer to the first character in the string,
not a pointer to the string itself. (So I think this should have a path of
length 1 containing `LValuePathEntry{.ArrayIndex = 0}`.)
================
Comment at: lib/AST/ExprConstant.cpp:5861-5863
+ Result.addArray(
+ Info, E,
+ cast<ConstantArrayType>(Str->getType()->getAsArrayTypeUnsafe()));
----------------
(This won't be necessary if/when `EvaluateInContext` returns a pointer to the
first character in the string.)
================
Comment at: lib/AST/ExprConstant.cpp:7616
+ Info.Ctx, Info.CurrentCall->CurSourceLocExprScope.getDefaultExpr());
+ return Success(Evaluated.getInt(), E);
+}
----------------
Consider dropping the `.getInt()` here? (Then this version and the pointer
version will be identical, and we could consider moving this to
`ExprEvaluatorBase`.)
================
Comment at: lib/AST/ExprConstant.cpp:11286
+ case Expr::SourceLocExprClass:
// GCC considers the GNU __null value to be an integral constant
expression.
return NoDiag();
----------------
This comment is getting detached from its case label. Maybe just remove it? It
seems reasonably obvious that `__null` should be an ICE.
================
Comment at: lib/AST/ExprConstant.cpp:3370-3371
+
+ assert((!Base || !isa<SourceLocExpr>(Base)) &&
+ "Base should have already been transformed into a StringLiteral");
+
----------------
EricWF wrote:
> rsmith wrote:
> > There are lots of forms of expression that cannot be the base of an
> > `LValue` (see the list above `LValueExprEvaluator` for the expression forms
> > that *can* be the base of an `LValue`); is it important to give this one
> > special treatment?
> Because a `SourceLocExpr` *can* be the base of an `LValue`. But the way
> that's supported is by transforming the `SourceLocExpr` into a
> `StringLiteral`.
I don't agree: a `SourceLocExpr` cannot be the base of an `LValue`. It is
evaluated into something else that can be (a `StringLiteral`), but it itself
cannot be (and this is in no way unusual; that's probably true of most `Expr`
nodes). I think this is a remnant of an earlier design?
================
Comment at: lib/CodeGen/CGExpr.cpp:3241-3281
Address CodeGenFunction::EmitArrayToPointerDecay(const Expr *E,
LValueBaseInfo *BaseInfo,
TBAAAccessInfo *TBAAInfo) {
- assert(E->getType()->isArrayType() &&
+ LValue LV = EmitLValue(E);
+ return EmitArrayToPointerDecay(E, E->getType(), LV, BaseInfo, TBAAInfo);
+}
+
----------------
This change should be unnecessary after changing `EvaluateInContext` to decay
the pointer.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:623-634
+
+ if (SLE->isIntType())
+ return Builder.getInt(Evaluated.getInt());
+
+ // If we're not building an int, then we're building a string literal.
+ const APValue::LValueBase &Base = Evaluated.getLValueBase();
+ const StringLiteral *Str = cast<StringLiteral>(Base.get<const Expr *>());
----------------
This can be simplified to:
```
return ConstantEmitter(CGF.CGM, CGF).emitAbstract(SLE->getLocation(),
Evaluated, SLE->getType());
```
(once `EvaluateInContext` decays the pointer).
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:971-977
+void ASTStmtReader::VisitSourceLocExpr(SourceLocExpr *E) {
+ VisitExpr(E);
+ E->setParentContext(ReadDeclAs<DeclContext>());
+ E->setBeginLoc(ReadSourceLocation());
+ E->setEndLoc(ReadSourceLocation());
+ E->setIdentKind(static_cast<SourceLocExpr::IdentKind>(Record.readInt()));
+}
----------------
Please directly set the fields here and remove the setters; we generally don't
want AST nodes to have setter functions, as they're supposed to be immutable
after creation. (Deserialization is an exception to this only because it is
creating the nodes itself.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D37035/new/
https://reviews.llvm.org/D37035
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
