[cfe-commits] [PATCH] Remove PotentiallyPotentiallyEvaluated

[John McCall]

On Jan 18, 2012, at 8:57 PM, Eli Friedman wrote:
> 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); }
> };

Heh.  Yeah, this bit of standard changed way too much.

> Anyway, new version of the patch attached which passes all the
> regression tests, using your suggestion.

--- 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);

Please update the comment to discuss / point to a discussion of all this.
There's another instance of this elsewhere in your patch.  It's just good to
make sure that code readers know that we understand that the context
isn't actually always unevaluated.

-  // 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,

This reads wrong.  I think you're going for something like "a field which is
not a member of the containing class (if there is one)"?

+namespace {
+  // Handle the case where we conclude a potentially-potentially-evaluated
+  // expression is actually evaluated.
+  class PPEToPETransform :
+    public TreeTransform<PPEToPETransform> {

"Potentially-potentially-evaluated" is no longer a term of art used anywhere
else in the source, so you should at least describe it and mention the
typeid and sizeof cases here.

Also, I'm always a fan of "super" typedefs in cases like this where superclass
calls are common and the superclass has a complicated type.

Also, would you mind measuring the code-size increase from adding a new
instance of TreeTransform?

+    // Error on DeclRefExprs referring to FieldDecls.
+    // FIXME: This actually isn't supposed to be an error!
+    ExprResult TransformDeclRefExpr(DeclRefExpr *E) {

What's up with this FIXME?

Otherwise looks good.

John.