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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
