[cfe-commits] [PATCH] Remove PotentiallyPotentiallyEvaluated
Eli Friedman
eli.friedman at gmail.com
Wed Jan 18 20:57:41 PST 2012
On Wed, Jan 18, 2012 at 8:02 PM, John McCall <rjmccall at apple.com> wrote:
> On Jan 18, 2012, at 7:42 PM, Eli Friedman wrote:
>> On Wed, Jan 18, 2012 at 6:58 PM, John McCall <rjmccall at apple.com> wrote:
>>> struct V { virtual void foo(); };
>>> V &foo(int);
>>> struct A { int x; };
>>> void test() { (void) typeid(foo(A::x)); }
>>
>> Yes, that's what CXX/expr/expr.prim/expr.prim.general/p12-0x.cpp looks like.
>
> I had an old checkout.
>
>>> But it's not possible to notice that it's ill-formed except by walking back
>>> over the operand to typeid and retroactively complaining about the use
>>> of A::x in a PE context.
>>
>> Well, you can sort of assume that it's unevaluated and buffer an error
>> for if the context ends up potentially-evaluated. I actually
>> committed some code that tries to do that, but I don't think it
>> actually works correctly in dependent contexts.
>
> It's really better to just do a second pass over the AST, probably as part
> of your transform.
>
>> Anyway, you can distinguish this case from member pointer formation
>> easily if you have the entire expression tree available; the issue is
>> that the whole tree isn't really available when we're transforming a
>> DeclRefExpr inside TreeTransform.
>
> Just teach your transform to ignore unary &s of pointer-to-member type.
>
>>> Memo to Doug: our example doesn't quite work. The rule is that it's
>>> a member expression if 'this' is available and it's either PE *or* the
>>> context is a (non-strict) subclass of the declaring class of the field.
>>> In any context where it would be potentially valid as a member
>>> expression, it is one. The problem is in other contexts, like the test
>>> above, where there's an "ambiguity" between being ill-formed
>>> and being an abstract member reference. Clearly this should be
>>> resolved in favor of being an abstract member reference, with
>>> this extra check being required after deciding that something is
>>> really PE. Up to you whether this rises to the level of a drafting error.
>>
>> The standard is completely clear; granted, it's a bit stupid that
>> sizeof(A::x) is legal in some, but not all contexts.
>
> Formally, the standard states that an expression should be rewritten
> (and accordingly analyzed differently) if a predicate holds when the
> value of that predicate depends on whether or not the expression is
> so rewritten. That is unsound.
>
> I mis-stated the test case; it should be:
>
> struct V { virtual void foo(); };
> V &foo(int &);
> struct A { int x; };
> struct B {
> void test() { (void) typeid(foo(A::x)); }
> };
>
> The analysis which leads to deciding that the typeid operand is PE
> relies on the assumption that the typeid operand is not PE.
Oh, OK. I got a little confused; clang currently follows the rules
from before the resolution of DR1017, which makes the following
outright illegal:
struct A { int x; };
struct B {
void test() { (void) sizeof(A::x); }
};
Anyway, new version of the patch attached which passes all the
regression tests, using your suggestion.
-Eli
-------------- next part --------------
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 148419)
+++ include/clang/Sema/Sema.h (working copy)
@@ -511,21 +511,6 @@
/// call was found yet.
bool ObjCShouldCallSuperFinalize;
- /// \brief The set of declarations that have been referenced within
- /// a potentially evaluated expression.
- typedef SmallVector<std::pair<SourceLocation, Decl *>, 10>
- PotentiallyReferencedDecls;
-
- /// \brief A set of diagnostics that may be emitted.
- typedef SmallVector<std::pair<SourceLocation, PartialDiagnostic>, 10>
- PotentiallyEmittedDiagnostics;
-
- typedef SmallVector<sema::DelayedDiagnostic, 10>
- PotentiallyEmittedDelayedDiag;
-
- typedef SmallVector<sema::PossiblyUnreachableDiag, 10>
- PotentiallyEmittedPossiblyUnreachableDiag;
-
/// \brief Describes how the expressions currently being parsed are
/// evaluated at run-time, if at all.
enum ExpressionEvaluationContext {
@@ -546,13 +531,6 @@
/// expression at run time.
PotentiallyEvaluated,
- /// \brief The current expression may be potentially evaluated or it may
- /// be unevaluated, but it is impossible to tell from the lexical context.
- /// This evaluation context is used primary for the operand of the C++
- /// \c typeid expression, whose argument is potentially evaluated only when
- /// it is an lvalue of polymorphic class type (C++ [basic.def.odr]p2).
- PotentiallyPotentiallyEvaluated,
-
/// \brief The current expression is potentially evaluated, but any
/// declarations referenced inside that expression are only used if
/// in fact the current expression is used.
@@ -577,43 +555,11 @@
/// this expression evaluation context.
unsigned NumCleanupObjects;
- /// \brief The set of declarations referenced within a
- /// potentially potentially-evaluated context.
- ///
- /// When leaving a potentially potentially-evaluated context, each
- /// of these elements will be as referenced if the corresponding
- /// potentially potentially evaluated expression is potentially
- /// evaluated.
- PotentiallyReferencedDecls *PotentiallyReferenced;
-
- // There are three kinds of diagnostics we care about in
- // PotentiallyPotentiallyEvaluated contexts: regular Diag diagnostics,
- // DelayedDiagnostics, and DiagRuntimeBehavior diagnostics.
- PotentiallyEmittedDiagnostics *SavedDiag;
- PotentiallyEmittedDelayedDiag *SavedDelayedDiag;
- PotentiallyEmittedPossiblyUnreachableDiag *SavedRuntimeDiag;
-
ExpressionEvaluationContextRecord(ExpressionEvaluationContext Context,
unsigned NumCleanupObjects,
bool ParentNeedsCleanups)
: Context(Context), ParentNeedsCleanups(ParentNeedsCleanups),
- NumCleanupObjects(NumCleanupObjects),
- PotentiallyReferenced(0), SavedDiag(0), SavedDelayedDiag(0),
- SavedRuntimeDiag(0) { }
-
- void addReferencedDecl(SourceLocation Loc, Decl *Decl) {
- if (!PotentiallyReferenced)
- PotentiallyReferenced = new PotentiallyReferencedDecls;
- PotentiallyReferenced->push_back(std::make_pair(Loc, Decl));
- }
-
- void addDiagnostic(SourceLocation Loc, const PartialDiagnostic &PD);
-
- void addRuntimeDiagnostic(const sema::PossiblyUnreachableDiag &PUD);
-
- void addDelayedDiagnostic(const sema::DelayedDiagnostic &DD);
-
- void Destroy();
+ NumCleanupObjects(NumCleanupObjects) { }
};
/// A stack of expression evaluation contexts.
@@ -2335,6 +2281,8 @@
void DiscardCleanupsInEvaluationContext();
+ ExprResult TranformToPotentiallyEvaluated(Expr *E);
+
void MarkDeclarationReferenced(SourceLocation Loc, Decl *D);
void MarkDeclarationsReferencedInType(SourceLocation Loc, QualType T);
void MarkDeclarationsReferencedInExpr(Expr *E);
@@ -3142,8 +3090,7 @@
QualType getCurrentThisType();
/// \brief Make sure the value of 'this' is actually available in the current
- /// context, if it is a potentially evaluated context. This check can be
- /// delayed in PotentiallyPotentiallyEvaluated contexts.
+ /// context, if it is a potentially evaluated context.
void CheckCXXThisCapture(SourceLocation Loc);
/// ActOnCXXBoolLiteral - Parse {true,false} literals.
Index: lib/Sema/TreeTransform.h
===================================================================
--- lib/Sema/TreeTransform.h (revision 148419)
+++ lib/Sema/TreeTransform.h (working copy)
@@ -6881,8 +6881,7 @@
// We don't know whether the expression is potentially evaluated until
// after we perform semantic analysis, so the expression is potentially
// potentially evaluated.
- EnterExpressionEvaluationContext Unevaluated(SemaRef,
- Sema::PotentiallyPotentiallyEvaluated);
+ EnterExpressionEvaluationContext Unevaluated(SemaRef, Sema::Unevaluated);
ExprResult SubExpr = getDerived().TransformExpr(E->getExprOperand());
if (SubExpr.isInvalid())
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp (revision 148419)
+++ lib/Sema/SemaExprCXX.cpp (working copy)
@@ -310,7 +310,6 @@
SourceLocation TypeidLoc,
Expr *E,
SourceLocation RParenLoc) {
- bool isUnevaluatedOperand = true;
if (E && !E->isTypeDependent()) {
if (E->getType()->isPlaceholderType()) {
ExprResult result = CheckPlaceholderExpr(E);
@@ -332,7 +331,11 @@
// polymorphic class type [...] [the] expression is an unevaluated
// operand. [...]
if (RecordD->isPolymorphic() && E->Classify(Context).isGLValue()) {
- isUnevaluatedOperand = false;
+ // The subexpression is potentially evaluated; switch the context
+ // and recheck the subexpression.
+ ExprResult Result = TranformToPotentiallyEvaluated(E);
+ if (Result.isInvalid()) return ExprError();
+ E = Result.take();
// We require a vtable to query the type at run time.
MarkVTableUsed(TypeidLoc, RecordD);
@@ -352,12 +355,6 @@
}
}
- // If this is an unevaluated operand, clear out the set of
- // declaration references we have been computing and eliminate any
- // temporaries introduced in its computation.
- if (isUnevaluatedOperand)
- ExprEvalContexts.back().Context = Unevaluated;
-
return Owned(new (Context) CXXTypeidExpr(TypeInfoType.withConst(),
E,
SourceRange(TypeidLoc, RParenLoc)));
@@ -695,14 +692,7 @@
continue;
}
// This context can't implicitly capture 'this'; fail out.
- // (We need to delay the diagnostic in the
- // PotentiallyPotentiallyEvaluated case because it doesn't apply to
- // unevaluated contexts.)
- if (ExprEvalContexts.back().Context == PotentiallyPotentiallyEvaluated)
- ExprEvalContexts.back()
- .addDiagnostic(Loc, PDiag(diag::err_implicit_this_capture));
- else
- Diag(Loc, diag::err_implicit_this_capture);
+ Diag(Loc, diag::err_implicit_this_capture);
return;
}
break;
Index: lib/Sema/SemaType.cpp
===================================================================
--- lib/Sema/SemaType.cpp (revision 148419)
+++ lib/Sema/SemaType.cpp (working copy)
@@ -1057,40 +1057,23 @@
} else if (type->isObjCARCImplicitlyUnretainedType()) {
implicitLifetime = Qualifiers::OCL_ExplicitNone;
- // If we are in an unevaluated context, like sizeof, assume Autoreleasing and
- // don't give error.
+ // If we are in an unevaluated context, like sizeof, skip adding a
+ // qualification.
} else if (S.ExprEvalContexts.back().Context == Sema::Unevaluated ||
S.ExprEvalContexts.back().Context == Sema::ConstantEvaluated) {
- implicitLifetime = Qualifiers::OCL_Autoreleasing;
+ return type;
// If that failed, give an error and recover using __autoreleasing.
} else {
// These types can show up in private ivars in system headers, so
// we need this to not be an error in those cases. Instead we
// want to delay.
- //
- // Also, make sure we delay appropriately in
- // PotentiallyPotentiallyEvaluated contexts.
if (S.DelayedDiagnostics.shouldDelayDiagnostics()) {
- if (S.ExprEvalContexts.back().Context ==
- Sema::PotentiallyPotentiallyEvaluated) {
- S.ExprEvalContexts.back().addDelayedDiagnostic(
- sema::DelayedDiagnostic::makeForbiddenType(loc,
- diag::err_arc_indirect_no_ownership, type, isReference));
- } else {
- S.DelayedDiagnostics.add(
- sema::DelayedDiagnostic::makeForbiddenType(loc,
- diag::err_arc_indirect_no_ownership, type, isReference));
- }
+ S.DelayedDiagnostics.add(
+ sema::DelayedDiagnostic::makeForbiddenType(loc,
+ diag::err_arc_indirect_no_ownership, type, isReference));
} else {
- if (S.ExprEvalContexts.back().Context ==
- Sema::PotentiallyPotentiallyEvaluated) {
- S.ExprEvalContexts.back().addDiagnostic(loc,
- S.PDiag(diag::err_arc_indirect_no_ownership)
- << type << isReference);
- } else {
- S.Diag(loc, diag::err_arc_indirect_no_ownership) << type << isReference;
- }
+ S.Diag(loc, diag::err_arc_indirect_no_ownership) << type << isReference;
}
implicitLifetime = Qualifiers::OCL_Autoreleasing;
}
Index: lib/Sema/SemaExprMember.cpp
===================================================================
--- lib/Sema/SemaExprMember.cpp (revision 148419)
+++ lib/Sema/SemaExprMember.cpp (working copy)
@@ -74,15 +74,10 @@
/// context is not an instance method.
IMA_Unresolved_StaticContext,
- // The reference is an instance data member access, which is allowed
- // because we're in C++11 mode and the context is unevaluated.
+ // The reference is to a field which is not an instance data member, which
+ // is allowed because we're in C++11 mode and the context is unevaluated.
IMA_Field_Uneval_StaticContext,
- // The reference is an instance data member access, which may be allowed
- // because we're in C++11 mode and the context may be unevaluated
- // (i.e. the context is PotentiallyPotentiallyEvaluated).
- IMA_Field_PPE_StaticContext,
-
/// All possible referrents are instance members and the current
/// context is not an instance method.
IMA_Error_StaticContext,
@@ -159,8 +154,6 @@
= SemaRef.ExprEvalContexts.back();
if (record.Context == Sema::Unevaluated)
return IMA_Field_Uneval_StaticContext;
- if (record.Context == Sema::PotentiallyPotentiallyEvaluated)
- return IMA_Field_PPE_StaticContext;
}
return IMA_Error_StaticContext;
@@ -196,8 +189,7 @@
static void DiagnoseInstanceReference(Sema &SemaRef,
const CXXScopeSpec &SS,
NamedDecl *rep,
- const DeclarationNameInfo &nameInfo,
- bool DelayPPEDiag = false) {
+ const DeclarationNameInfo &nameInfo) {
SourceLocation Loc = nameInfo.getLoc();
SourceRange Range(Loc);
if (SS.isSet()) Range.setBegin(SS.getRange().getBegin());
@@ -206,29 +198,17 @@
if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(SemaRef.CurContext)) {
if (MD->isStatic()) {
// "invalid use of member 'x' in static member function"
- if (DelayPPEDiag)
- SemaRef.ExprEvalContexts.back().addDiagnostic(Loc,
- SemaRef.PDiag(diag::err_invalid_member_use_in_static_method)
- << Range << nameInfo.getName());
- else
- SemaRef.Diag(Loc, diag::err_invalid_member_use_in_static_method)
- << Range << nameInfo.getName();
+ SemaRef.Diag(Loc, diag::err_invalid_member_use_in_static_method)
+ << Range << nameInfo.getName();
return;
}
}
- if (DelayPPEDiag)
- SemaRef.ExprEvalContexts.back().addDiagnostic(Loc,
- SemaRef.PDiag(diag::err_invalid_non_static_member_use)
- << nameInfo.getName() << Range);
- else
- SemaRef.Diag(Loc, diag::err_invalid_non_static_member_use)
- << nameInfo.getName() << Range;
+ SemaRef.Diag(Loc, diag::err_invalid_non_static_member_use)
+ << nameInfo.getName() << Range;
return;
}
- assert(!DelayPPEDiag && "Only need to delay diagnostic for fields");
-
SemaRef.Diag(Loc, diag::err_member_call_without_object) << Range;
}
@@ -246,11 +226,6 @@
case IMA_Unresolved:
return BuildImplicitMemberExpr(SS, R, TemplateArgs, false);
- case IMA_Field_PPE_StaticContext:
- DiagnoseInstanceReference(*this, SS, R.getRepresentativeDecl(),
- R.getLookupNameInfo(), /*DelayPPEDiag*/true);
- // FALL-THROUGH
-
case IMA_Static:
case IMA_Mixed_StaticContext:
case IMA_Unresolved_StaticContext:
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 148419)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -40,6 +40,7 @@
#include "clang/Sema/ParsedTemplate.h"
#include "clang/Sema/SemaFixItUtils.h"
#include "clang/Sema/Template.h"
+#include "TreeTransform.h"
using namespace clang;
using namespace sema;
@@ -1208,10 +1209,6 @@
case Sema::PotentiallyEvaluated:
case Sema::PotentiallyEvaluatedIfUsed:
break;
-
- case Sema::PotentiallyPotentiallyEvaluated:
- // FIXME: delay these!
- break;
}
// Don't diagnose about capture if we're not actually in code right
@@ -9361,39 +9358,52 @@
return false;
}
-void Sema::ExpressionEvaluationContextRecord::Destroy() {
- delete PotentiallyReferenced;
- delete SavedDiag;
- delete SavedRuntimeDiag;
- delete SavedDelayedDiag;
- PotentiallyReferenced = 0;
- SavedDiag = 0;
- SavedRuntimeDiag = 0;
- SavedDelayedDiag = 0;
-}
+namespace {
+ // Handle the case where we conclude a potentially-potentially-evaluated
+ // expression is actually evaluated.
+ class PPEToPETransform :
+ public TreeTransform<PPEToPETransform> {
+ public:
+ PPEToPETransform(Sema &SemaRef) :
+ TreeTransform<PPEToPETransform>(SemaRef) {
+ }
-void Sema::ExpressionEvaluationContextRecord::addDiagnostic(
- SourceLocation Loc, const PartialDiagnostic &PD) {
- if (!SavedDiag)
- SavedDiag = new PotentiallyEmittedDiagnostics;
- SavedDiag->push_back(std::make_pair(Loc, PD));
-}
+ // Make sure we redo semantic analysis
+ bool AlwaysRebuild() { return true; }
-void Sema::ExpressionEvaluationContextRecord::addRuntimeDiagnostic(
- const sema::PossiblyUnreachableDiag &PUD) {
- if (!SavedRuntimeDiag)
- SavedRuntimeDiag = new PotentiallyEmittedPossiblyUnreachableDiag;
- SavedRuntimeDiag->push_back(PUD);
+ // We need to special-case DeclRefExprs referring to FieldDecls which
+ // are not part of a member pointer formation; normal TreeTransforming
+ // doesn't catch this case because of the way we represent them in the AST.
+ //
+ // Filter out member pointer formation
+ ExprResult TransformUnaryOperator(UnaryOperator *E) {
+ if (E->getOpcode() == UO_AddrOf && E->getType()->isMemberPointerType())
+ return E;
+
+ return TreeTransform<PPEToPETransform>::TransformUnaryOperator(E);
+ }
+
+ // Error on DeclRefExprs referring to FieldDecls.
+ // FIXME: This actually isn't supposed to be an error!
+ ExprResult TransformDeclRefExpr(DeclRefExpr *E) {
+ if (isa<FieldDecl>(E->getDecl()))
+ return SemaRef.Diag(E->getLocation(),
+ diag::err_invalid_non_static_member_use)
+ << E->getDecl() << E->getSourceRange();
+
+ return TreeTransform<PPEToPETransform>::TransformDeclRefExpr(E);
+ }
+ };
}
-void Sema::ExpressionEvaluationContextRecord::addDelayedDiagnostic(
- const sema::DelayedDiagnostic &DD) {
- if (!SavedDelayedDiag)
- SavedDelayedDiag = new PotentiallyEmittedDelayedDiag;
- SavedDelayedDiag->push_back(DD);
+ExprResult Sema::TranformToPotentiallyEvaluated(Expr *E) {
+ ExprEvalContexts.back().Context =
+ ExprEvalContexts[ExprEvalContexts.size()-2].Context;
+ if (ExprEvalContexts.back().Context == Unevaluated)
+ return E;
+ return PPEToPETransform(*this).TransformExpr(E);
}
-
void
Sema::PushExpressionEvaluationContext(ExpressionEvaluationContext NewContext) {
ExprEvalContexts.push_back(
@@ -9408,46 +9418,6 @@
ExpressionEvaluationContextRecord Rec = ExprEvalContexts.back();
ExprEvalContexts.pop_back();
- if (Rec.Context == PotentiallyPotentiallyEvaluated) {
- if (Rec.PotentiallyReferenced) {
- // Mark any remaining declarations in the current position of the stack
- // as "referenced". If they were not meant to be referenced, semantic
- // analysis would have eliminated them (e.g., in ActOnCXXTypeId).
- for (PotentiallyReferencedDecls::iterator
- I = Rec.PotentiallyReferenced->begin(),
- IEnd = Rec.PotentiallyReferenced->end();
- I != IEnd; ++I)
- MarkDeclarationReferenced(I->first, I->second);
- }
-
- if (Rec.SavedDiag) {
- // Emit any pending diagnostics.
- for (PotentiallyEmittedDiagnostics::iterator
- I = Rec.SavedDiag->begin(),
- IEnd = Rec.SavedDiag->end();
- I != IEnd; ++I)
- Diag(I->first, I->second);
- }
-
- if (Rec.SavedDelayedDiag) {
- // Emit any pending delayed diagnostics.
- for (PotentiallyEmittedDelayedDiag::iterator
- I = Rec.SavedDelayedDiag->begin(),
- IEnd = Rec.SavedDelayedDiag->end();
- I != IEnd; ++I)
- DelayedDiagnostics.add(*I);
- }
-
- if (Rec.SavedRuntimeDiag) {
- // Emit any pending runtime diagnostics.
- for (PotentiallyEmittedPossiblyUnreachableDiag::iterator
- I = Rec.SavedRuntimeDiag->begin(),
- IEnd = Rec.SavedRuntimeDiag->end();
- I != IEnd; ++I)
- FunctionScopes.back()->PossiblyUnreachableDiags.push_back(*I);
- }
- }
-
// When are coming out of an unevaluated context, clear out any
// temporaries that we may have created as part of the evaluation of
// the expression in that context: they aren't relevant because they
@@ -9461,9 +9431,6 @@
} else {
ExprNeedsCleanups |= Rec.ParentNeedsCleanups;
}
-
- // Destroy the popped expression evaluation record.
- Rec.Destroy();
}
void Sema::DiscardCleanupsInEvaluationContext() {
@@ -9519,13 +9486,6 @@
// "used"; handle this below.
break;
- case PotentiallyPotentiallyEvaluated:
- // We are in an expression that may be potentially evaluated; queue this
- // declaration reference until we know whether the expression is
- // potentially evaluated.
- ExprEvalContexts.back().addReferencedDecl(Loc, D);
- return;
-
case PotentiallyEvaluatedIfUsed:
// Referenced declarations will only be used if the construct in the
// containing expression is used.
@@ -9809,11 +9769,6 @@
Diag(Loc, PD);
return true;
-
- case PotentiallyPotentiallyEvaluated:
- ExprEvalContexts.back().addRuntimeDiagnostic(
- sema::PossiblyUnreachableDiag(PD, Loc, Statement));
- break;
}
return false;
Index: lib/Parse/ParseExprCXX.cpp
===================================================================
--- lib/Parse/ParseExprCXX.cpp (revision 148419)
+++ lib/Parse/ParseExprCXX.cpp (working copy)
@@ -926,8 +926,7 @@
// Note that we can't tell whether the expression is an lvalue of a
// polymorphic class type until after we've parsed the expression, so
// we the expression is potentially potentially evaluated.
- EnterExpressionEvaluationContext Unevaluated(Actions,
- Sema::PotentiallyPotentiallyEvaluated);
+ EnterExpressionEvaluationContext Unevaluated(Actions, Sema::Unevaluated);
Result = ParseExpression();
// Match the ')'.
More information about the cfe-commits
mailing list