[PATCH] D22476: [AST] Make MemberExpr non-dependent according to core issue 224

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.


================
Comment at: lib/AST/Expr.cpp:1413
+    for (auto *EnableIf : Method->specific_attrs<EnableIfAttr>()) {
+      if (EnableIf->getCond()->isTypeDependent())
+        return false;
----------------
rsmith wrote:
> You should presumably be checking `isValueDependent` here.
> 
> I also don't see any test changes covering the handling of this attribute, and it's not completely clear to me why a dependent enable_if attribute would cause a member to be treated as not being a member of the current instantiation. This should probably be grouped with the check for a member with a dependent type down on line 1458, not as part of the determination of whether we have a member of the current instantiation.
Thanks, I will move the check down.

A test for this is in test/SemaCXX/enable_if.cpp:
```
template <typename T> class C {
  void f() __attribute__((enable_if(T::expr == 0, ""))) {}
  void g() { f(); }
};
```
If I would not check for value dependent attributes, then the call to f would be considered non-type dependent and produce the warnings
```
error: 'error' diagnostics seen but not expected: 
  File test/SemaCXX/enable_if.cpp Line 116: no matching member function for call to 'f'
error: 'note' diagnostics seen but not expected: 
  File test/SemaCXX/enable_if.cpp Line 115: candidate disabled: <no message provided>
2 errors generated.
```
We need to keep f() type-dependent until the template is instantiated.




================
Comment at: lib/AST/Expr.cpp:1456
+  // the field is type dependent. (14.6.2.1 [temp.dep.type])
+  if (E->isTypeDependent()
+   && IsMemberOfCurrentInstantiation(base, memberdecl)
----------------
rsmith wrote:
> rsmith wrote:
> > Please clang-format this.
> The `E->isTypeDependent()` test should be in `isMemberOfCurrentInstantiation` -- right now, that function is determining whether we have a member of the current instantiation or a member of a non-dependent type.
I will move it there.


================
Comment at: lib/AST/Expr.cpp:1456-1458
+  if (E->isTypeDependent()
+   && IsMemberOfCurrentInstantiation(base, memberdecl)
+   && !memberdecl->getType()->isDependentType()) {
----------------
mgehre wrote:
> rsmith wrote:
> > rsmith wrote:
> > > Please clang-format this.
> > The `E->isTypeDependent()` test should be in `isMemberOfCurrentInstantiation` -- right now, that function is determining whether we have a member of the current instantiation or a member of a non-dependent type.
> I will move it there.
Sorry, done.


================
Comment at: test/CXX/temp/temp.res/temp.dep/temp.dep.expr/p5.cpp:14-15
+      i = nullptr;
+      (*this).i = j; // FIXME {{incompatible type}}
+      this->i = j; // FIXME {{incompatible type}}
+      A::i = j; // expected-error {{incompatible type}}
----------------
rsmith wrote:
> Do you know why we don't diagnose this?
This patch adds support for the MemberExpr, e.g.
```
i = j; // expected-error {{incompatible type}}
```
which before this patch had the AST
```
  |   |   |-BinaryOperator 0x3449f50 <line:10:5, col:9> '<dependent type>' '='
  |   |   | |-MemberExpr 0x3449ef0 <col:5> 'int *' lvalue ->i 0x3449950
  |   |   | | `-CXXThisExpr 0x3449ed8 <col:5> 'A<T, I> *' this
  |   |   | `-DeclRefExpr 0x3449f28 <col:9> 'char *' lvalue Var 0x3449e30 'j' 'char *'
```

Not yet fixed are CXXDependentScopeMemberExpr, i.e.
```
(*this).i = j;               // FIXME {{incompatible type}}
  |   |   | - BinaryOperator 0x8cfbd0 <line:12:5, col:17> '<dependent type>' '='
  |   |   | |-CXXDependentScopeMemberExpr 0x8cfb50 <col:5, col:13> '<dependent type>' lvalue .i
  |   |   | | `-ParenExpr 0x8cfb30 <col:5, col:11> '<dependent type>'
  |   |   | |   `-UnaryOperator 0x8cfb10 <col:6, col:7> '<dependent type>' prefix '*'
  |   |   | |     `-CXXThisExpr 0x8cfaf8 <col:7> 'A<T, I> *' this
  |   |   | `-DeclRefExpr 0x8cfba8 <col:17> 'char *' lvalue Var 0x8a5a08 'j' 'char *'
```


```
this->i = j;                 // FIXME {{incompatible type}}
  |   |   |-BinaryOperator 0x8cfc90 <line:13:5, col:15> '<dependent type>' '='
  |   |   | |-CXXDependentScopeMemberExpr 0x8cfc10 <col:5, col:11> '<dependent type>' lvalue ->i
  |   |   | | `-CXXThisExpr 0x8cfbf8 <col:5> 'A<T, I> *' this
  |   |   | `-DeclRefExpr 0x8cfc68 <col:15> 'char *' lvalue Var 0x8a5a08 'j' 'char *'
```


https://reviews.llvm.org/D22476