[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13824
 SourceLocation OpLoc) {
-  if (Op->isTypeDependent())
+  if (Op->isTypeDependent() && !Op->getType()->isPointerType())
 return S.Context.DependentTy;

aaron.ballman wrote:
> One thing that makes me a bit uncomfortable is that the logic for these used 
> to be far more understandable when it was just checking for a dependent type. 
> Now we need to sprinkle "and not a pointer type" in places, but it's not 
> particularly clear as to why for a naïve reader of the code.
> 
> I wonder if we want some sort of type predicate `isDeferrablyDependentType()` 
> or something along those lines, or whether that's not really plausible? 
> Basically, I'm hoping to find a way that, as a code reviewer, I can more 
> easily spot places where `isTypeDependent()` should really be caring about 
> type dependent pointers as a special case.
Right, so in this function `Op->isTypeDependent()` is always used to bail out 
from type checking. The structure is typically:

```
if (E->isTypeDependent()) {
  // bail out
}
// Actual type checking on provable types.
if (E->hasSomeTypeProperty1()) {
  // OK case, do some property1-specific checking
} else if (E->hasSomeTypeProperty2()) {
  // OK case, do some property2-specific checking
} else {
  // Emit some error
}
```

My original approach was to do:

```
if (E->isTypeDependent() && !E->isPointerType()) {
  // bail out
}
// Actual type checking on provable types or pointer types.
if (E->hasSomeTypeProperty1()) {
  // OK case, do some property1-specific checking
} else if (E->hasSomeTypeProperty2()) {
  // OK case, do some property2-specific checking
} else {
  // Emit some error
}
```

It turns out that in all cases, `hasSomeTypeProperty1` is actually 
`isPointerType` (or stuff like `isScalarType`, which includes pointers), so the 
current code is actually already checking for pointerness, so in a sense my new 
pointer type cheking is already included in subsequent checks, I'm not adding 
anything new.

But maybe another change like this one will be able to  prove more stuff about 
dependent types (e.g. a dependent type could have propery2 too ), so what about 
we only bail out on dependent types *after* we are done with the type checking:

```
if (E->isTypeDependent() && !E->isPointerType()) {
  // bail out
}
// Actual type checking on provable types or pointer types.
if (E->hasSomeTypeProperty1()) {
  // OK case, do some property1-specific checking
} else if (E->hasSomeTypeProperty2()) {
  // OK case, do some property2-specific checking
} else if (if (E->isTypeDependent()) {
  // bail out
} else {
  // Emit some error
}
```

This essentially goes from "If the type is not dependent, try to prove stuff 
about the type, else return error" to "try to prove stuff about the type, else 
if not dependent, return error".

I'm modified the code here to use this approach, what do you think ?





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 395410.
courbet added a comment.

Try to get rid of "dependent and not a pointer" checks checks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

Files:
  clang/include/clang/AST/Type.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/AST/ast-dump-expr-json.cpp
  clang/test/AST/ast-dump-expr.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/CXX/over/over.built/ast-20.cpp
  clang/test/CXX/over/over.built/ast.cpp
  clang/test/CXX/over/over.built/p10.cpp
  clang/test/CXX/over/over.built/p11.cpp
  clang/test/CXX/over/over.built/p13.cpp
  clang/test/CXX/over/over.built/p14.cpp
  clang/test/CXX/over/over.built/p18.cpp
  clang/test/CXX/over/over.built/p19.cpp
  clang/test/CXX/over/over.built/p20.cpp
  clang/test/CXX/over/over.built/p22.cpp
  clang/test/CXX/over/over.built/p23.cpp
  clang/test/CXX/over/over.built/p4.cpp
  clang/test/CXX/over/over.built/p5.cpp
  clang/test/CXX/over/over.built/p7.cpp
  clang/test/CXX/over/over.built/spaceship.cpp
  clang/test/Frontend/noderef_templates.cpp
  clang/test/SemaTemplate/dependent-type-identity.cpp

Index: clang/test/SemaTemplate/dependent-type-identity.cpp
===
--- clang/test/SemaTemplate/dependent-type-identity.cpp
+++ clang/test/SemaTemplate/dependent-type-identity.cpp
@@ -69,6 +69,16 @@
   void f8(typename N::X2::template apply *);
   void f8(typename N::X2::template apply *);
   void f8(typename ::Nalias::X2::template apply *); // expected-error{{redeclar}}
+
+  // (17.4.2): If an expression e is type-dependent (17.6.2.2), decltype(e)
+  // denotes a unique dependent type. Two such decltype-specifiers refer to the
+  // same type only if their expressions are equivalent (17.5.6.1)
+  T* a;
+  T* b;
+  using V = decltype(*a);
+  void f9(decltype(*a)); // expected-note{{previous}}
+  void f9(decltype(*b));
+  void f9(V); // expected-error{{redeclar}}
 };
 
 namespace PR6851 {
Index: clang/test/Frontend/noderef_templates.cpp
===
--- clang/test/Frontend/noderef_templates.cpp
+++ clang/test/Frontend/noderef_templates.cpp
@@ -3,8 +3,8 @@
 #define NODEREF __attribute__((noderef))
 
 template 
-int func(T NODEREF *a) { // expected-note 2 {{a declared here}}
-  return *a + 1; // expected-warning 2 {{dereferencing a; was declared with a 'noderef' type}}
+int func(T NODEREF *a) { // expected-note 3 {{a declared here}}
+  return *a + 1; // expected-warning 3 {{dereferencing a; was declared with a 'noderef' type}}
 }
 
 void func() {
Index: clang/test/CXX/over/over.built/spaceship.cpp
===
--- clang/test/CXX/over/over.built/spaceship.cpp
+++ clang/test/CXX/over/over.built/spaceship.cpp
@@ -11,11 +11,10 @@
 }
 
 template 
-void f(int i, float f, int* pi, T* pt, T t) {
+void f(int i, int* pi, T* pt, T t) {
   (void)(i <=> i);
-  (void)(i <=> f); // expected-error {{invalid argument type}}
-  (void)(i <=> pi); // expected-error {{invalid argument type}}
-  (void)(i <=> pt); // expected-error {{invalid argument type}}
+  (void)(i <=> pi); // expected-error {{comparison between pointer and integer}}
+  (void)(i <=> pt); // expected-error {{comparison between pointer and integer}}
   (void)(pi <=> pt);
   (void)(pi <=> t);
 }
Index: clang/test/CXX/over/over.built/p7.cpp
===
--- clang/test/CXX/over/over.built/p7.cpp
+++ clang/test/CXX/over/over.built/p7.cpp
@@ -3,10 +3,11 @@
 struct A{};
 
 template 
-void f(int* pi, A* pa, T* pt) {
+void f(int* pi, A* pa, T* pt, T t) {
   (void)*pi;
   (void)*pa;
   (void)*pt;
+  (void)*t;  // `T` might be a `U*`.
 }
 // expected-no-diagnostics
 
Index: clang/test/CXX/over/over.built/p5.cpp
===
--- clang/test/CXX/over/over.built/p5.cpp
+++ clang/test/CXX/over/over.built/p5.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)--i;
   (void)i--;
 
   (void)--b; // expected-error {{cannot decrement expression of type bool}}
   (void)b--; // expected-error {{cannot decrement expression of type bool}}
+
+  (void)--t;
+  (void)t--;
 }
 
Index: clang/test/CXX/over/over.built/p4.cpp
===
--- clang/test/CXX/over/over.built/p4.cpp
+++ clang/test/CXX/over/over.built/p4.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++17 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)++i;
   (void)i++;
 
   (void)++b; // expected-error {{ISO C++17 does not allow incrementing expression of type bool}}
   (void)b++; // expected-error {{ISO C++17 does not 

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:10546-10548
+  if (PointeeTy->isDependentType()) {
+return true;
+  }





Comment at: clang/lib/Sema/SemaExpr.cpp:13824
 SourceLocation OpLoc) {
-  if (Op->isTypeDependent())
+  if (Op->isTypeDependent() && !Op->getType()->isPointerType())
 return S.Context.DependentTy;

One thing that makes me a bit uncomfortable is that the logic for these used to 
be far more understandable when it was just checking for a dependent type. Now 
we need to sprinkle "and not a pointer type" in places, but it's not 
particularly clear as to why for a naïve reader of the code.

I wonder if we want some sort of type predicate `isDeferrablyDependentType()` 
or something along those lines, or whether that's not really plausible? 
Basically, I'm hoping to find a way that, as a code reviewer, I can more easily 
spot places where `isTypeDependent()` should really be caring about type 
dependent pointers as a special case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 385768.
courbet added a comment.

one more spaceship operator fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

Files:
  clang/include/clang/AST/Type.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/AST/ast-dump-expr-json.cpp
  clang/test/AST/ast-dump-expr.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/CXX/over/over.built/ast-20.cpp
  clang/test/CXX/over/over.built/ast.cpp
  clang/test/CXX/over/over.built/p10.cpp
  clang/test/CXX/over/over.built/p11.cpp
  clang/test/CXX/over/over.built/p13.cpp
  clang/test/CXX/over/over.built/p14.cpp
  clang/test/CXX/over/over.built/p18.cpp
  clang/test/CXX/over/over.built/p19.cpp
  clang/test/CXX/over/over.built/p20.cpp
  clang/test/CXX/over/over.built/p22.cpp
  clang/test/CXX/over/over.built/p23.cpp
  clang/test/CXX/over/over.built/p4.cpp
  clang/test/CXX/over/over.built/p5.cpp
  clang/test/CXX/over/over.built/p7.cpp
  clang/test/CXX/over/over.built/spaceship.cpp
  clang/test/Frontend/noderef_templates.cpp
  clang/test/SemaTemplate/dependent-type-identity.cpp

Index: clang/test/SemaTemplate/dependent-type-identity.cpp
===
--- clang/test/SemaTemplate/dependent-type-identity.cpp
+++ clang/test/SemaTemplate/dependent-type-identity.cpp
@@ -69,6 +69,16 @@
   void f8(typename N::X2::template apply *);
   void f8(typename N::X2::template apply *);
   void f8(typename ::Nalias::X2::template apply *); // expected-error{{redeclar}}
+
+  // (17.4.2): If an expression e is type-dependent (17.6.2.2), decltype(e)
+  // denotes a unique dependent type. Two such decltype-specifiers refer to the
+  // same type only if their expressions are equivalent (17.5.6.1)
+  T* a;
+  T* b;
+  using V = decltype(*a);
+  void f9(decltype(*a)); // expected-note{{previous}}
+  void f9(decltype(*b));
+  void f9(V); // expected-error{{redeclar}}
 };
 
 namespace PR6851 {
Index: clang/test/Frontend/noderef_templates.cpp
===
--- clang/test/Frontend/noderef_templates.cpp
+++ clang/test/Frontend/noderef_templates.cpp
@@ -3,8 +3,8 @@
 #define NODEREF __attribute__((noderef))
 
 template 
-int func(T NODEREF *a) { // expected-note 2 {{a declared here}}
-  return *a + 1; // expected-warning 2 {{dereferencing a; was declared with a 'noderef' type}}
+int func(T NODEREF *a) { // expected-note 3 {{a declared here}}
+  return *a + 1; // expected-warning 3 {{dereferencing a; was declared with a 'noderef' type}}
 }
 
 void func() {
Index: clang/test/CXX/over/over.built/spaceship.cpp
===
--- clang/test/CXX/over/over.built/spaceship.cpp
+++ clang/test/CXX/over/over.built/spaceship.cpp
@@ -11,11 +11,10 @@
 }
 
 template 
-void f(int i, float f, int* pi, T* pt, T t) {
+void f(int i, int* pi, T* pt, T t) {
   (void)(i <=> i);
-  (void)(i <=> f); // expected-error {{invalid argument type}}
-  (void)(i <=> pi); // expected-error {{invalid argument type}}
-  (void)(i <=> pt); // expected-error {{invalid argument type}}
+  (void)(i <=> pi); // expected-error {{comparison between pointer and integer}}
+  (void)(i <=> pt); // expected-error {{comparison between pointer and integer}}
   (void)(pi <=> pt);
   (void)(pi <=> t);
 }
Index: clang/test/CXX/over/over.built/p7.cpp
===
--- clang/test/CXX/over/over.built/p7.cpp
+++ clang/test/CXX/over/over.built/p7.cpp
@@ -3,10 +3,11 @@
 struct A{};
 
 template 
-void f(int* pi, A* pa, T* pt) {
+void f(int* pi, A* pa, T* pt, T t) {
   (void)*pi;
   (void)*pa;
   (void)*pt;
+  (void)*t;  // `T` might be a `U*`.
 }
 // expected-no-diagnostics
 
Index: clang/test/CXX/over/over.built/p5.cpp
===
--- clang/test/CXX/over/over.built/p5.cpp
+++ clang/test/CXX/over/over.built/p5.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)--i;
   (void)i--;
 
   (void)--b; // expected-error {{cannot decrement expression of type bool}}
   (void)b--; // expected-error {{cannot decrement expression of type bool}}
+
+  (void)--t;
+  (void)t--;
 }
 
Index: clang/test/CXX/over/over.built/p4.cpp
===
--- clang/test/CXX/over/over.built/p4.cpp
+++ clang/test/CXX/over/over.built/p4.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++17 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)++i;
   (void)i++;
 
   (void)++b; // expected-error {{ISO C++17 does not allow incrementing expression of type bool}}
   (void)b++; // expected-error {{ISO C++17 does not allow incrementing expression 

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

> Is this change observable in some way?

With the new changes, we are now catching more typing errors before 
instantiation. I've added more tests to show that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 384771.
courbet added a comment.

Implement the proposed changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

Files:
  clang/include/clang/AST/Type.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/AST/ast-dump-expr-json.cpp
  clang/test/AST/ast-dump-expr.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/CXX/over/over.built/ast.cpp
  clang/test/CXX/over/over.built/p10.cpp
  clang/test/CXX/over/over.built/p11.cpp
  clang/test/CXX/over/over.built/p13.cpp
  clang/test/CXX/over/over.built/p14.cpp
  clang/test/CXX/over/over.built/p18.cpp
  clang/test/CXX/over/over.built/p19.cpp
  clang/test/CXX/over/over.built/p20.cpp
  clang/test/CXX/over/over.built/p22.cpp
  clang/test/CXX/over/over.built/p23.cpp
  clang/test/CXX/over/over.built/p4.cpp
  clang/test/CXX/over/over.built/p5.cpp
  clang/test/CXX/over/over.built/p7.cpp
  clang/test/Frontend/noderef_templates.cpp
  clang/test/SemaTemplate/dependent-type-identity.cpp

Index: clang/test/SemaTemplate/dependent-type-identity.cpp
===
--- clang/test/SemaTemplate/dependent-type-identity.cpp
+++ clang/test/SemaTemplate/dependent-type-identity.cpp
@@ -69,6 +69,16 @@
   void f8(typename N::X2::template apply *);
   void f8(typename N::X2::template apply *);
   void f8(typename ::Nalias::X2::template apply *); // expected-error{{redeclar}}
+
+  // (17.4.2): If an expression e is type-dependent (17.6.2.2), decltype(e)
+  // denotes a unique dependent type. Two such decltype-specifiers refer to the
+  // same type only if their expressions are equivalent (17.5.6.1)
+  T* a;
+  T* b;
+  using V = decltype(*a);
+  void f9(decltype(*a)); // expected-note{{previous}}
+  void f9(decltype(*b));
+  void f9(V); // expected-error{{redeclar}}
 };
 
 namespace PR6851 {
Index: clang/test/Frontend/noderef_templates.cpp
===
--- clang/test/Frontend/noderef_templates.cpp
+++ clang/test/Frontend/noderef_templates.cpp
@@ -3,8 +3,8 @@
 #define NODEREF __attribute__((noderef))
 
 template 
-int func(T NODEREF *a) { // expected-note 2 {{a declared here}}
-  return *a + 1; // expected-warning 2 {{dereferencing a; was declared with a 'noderef' type}}
+int func(T NODEREF *a) { // expected-note 3 {{a declared here}}
+  return *a + 1; // expected-warning 3 {{dereferencing a; was declared with a 'noderef' type}}
 }
 
 void func() {
Index: clang/test/CXX/over/over.built/p7.cpp
===
--- clang/test/CXX/over/over.built/p7.cpp
+++ clang/test/CXX/over/over.built/p7.cpp
@@ -3,10 +3,11 @@
 struct A{};
 
 template 
-void f(int* pi, A* pa, T* pt) {
+void f(int* pi, A* pa, T* pt, T t) {
   (void)*pi;
   (void)*pa;
   (void)*pt;
+  (void)*t;  // `T` might be a `U*`.
 }
 // expected-no-diagnostics
 
Index: clang/test/CXX/over/over.built/p5.cpp
===
--- clang/test/CXX/over/over.built/p5.cpp
+++ clang/test/CXX/over/over.built/p5.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)--i;
   (void)i--;
 
   (void)--b; // expected-error {{cannot decrement expression of type bool}}
   (void)b--; // expected-error {{cannot decrement expression of type bool}}
+
+  (void)--t;
+  (void)t--;
 }
 
Index: clang/test/CXX/over/over.built/p4.cpp
===
--- clang/test/CXX/over/over.built/p4.cpp
+++ clang/test/CXX/over/over.built/p4.cpp
@@ -1,10 +1,14 @@
 // RUN: %clang_cc1 -std=c++17 -verify %s -Wno-tautological-compare
 
-void f(int i, bool b) {
+template 
+void f(int i, bool b, T t) {
   (void)++i;
   (void)i++;
 
   (void)++b; // expected-error {{ISO C++17 does not allow incrementing expression of type bool}}
   (void)b++; // expected-error {{ISO C++17 does not allow incrementing expression of type bool}}
+
+  (void)++t;
+  (void)t++;
 }
 
Index: clang/test/CXX/over/over.built/p23.cpp
===
--- clang/test/CXX/over/over.built/p23.cpp
+++ clang/test/CXX/over/over.built/p23.cpp
@@ -6,41 +6,41 @@
   f %= 3;  // expected-error {{invalid operands}}
   b %= 3;
   pi %= 3; // expected-error {{invalid operands}}
-  pt %= 3; // FIXME
+  pt %= 3; // expected-error {{invalid operands}}
   t %= 3;
 
   i &= 3;
   f &= 3;  // expected-error {{invalid operands}}
   b &= 3;
   pi &= 3; // expected-error {{invalid operands}}
-  pt &= 3; // FIXME
+  pt &= 3; // expected-error {{invalid operands}}
   t &= 3;
 
   i ^= 3;
   f ^= 3;  // expected-error {{invalid operands}}
   b ^= 3;
   pi ^= 3; // expected-error {{invalid operands}}
-  pt ^= 3; // FIXME
+  pt ^= 3; // expected-error 

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In D112453#3103515 , @rsmith wrote:

> I think this change is being made in an imperfect place, though. Instead of 
> putting a special case in here, how would we feel about making 
> `Type::isOverloadableType()` smarter, to return `false` for dependent types 
> that can't possibly be class or enum types (eg, dependent pointer types, 
> array types, and maybe more exotic things like function types, atomic types, 
> and complex types)? This would then apply to all operators, not only unary 
> `*`. Eg, we know the type of `` where `p` is `T*`, and we know the type of 
> `p + n` where  `p` is `T*` and  `n` is integral. That change might have a lot 
> more fallout, but it'd certainly be interesting to see what it breaks.

Thanks for the suggestion ! I've improved test coverage for unary operators in 
rG1427742750ed 
. There 
was a minor breakage with this change in `Sema::DefaultLvalueConversion` which 
did not handle dependant pointers.

Binary operators look like they break more things. Working on it now.

> Is this change observable in some way? We should include a test with this 
> change that demonstrates what effect it has. (The existing test in this patch 
> passes with or without the change.)

Yes, I agree. The idea of this test was just to check that adding this did not 
have side effects on how clang was handling what the standard mandates, but 
you've covered this in the first part of your answer, thanks.
I don't think it's observable directly (I'm observing the type using 
`getType()` directly in my case), so I'll add an AST dump test, thanks for the 
suggestion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think the idea of this change is OK. The key is that the dereference 
expression will still be type-dependent, even if we happen to actually know its 
type. (For an `Expr` that `isTypeDependent()`, the type produced by `getType()` 
isn't something the standard knows or cares about and is only used for our own 
type-checking-of-templates purposes.) The property we need to guarantee is 
that, if the `Expr` is valid, then the type is correct, and I think that holds 
here. However, I wouldn't want to guarantee that all parts of Clang get this 
right, and we might find some lurking bugs are exposed by this change.

I think this change is being made in an imperfect place, though. Instead of 
putting a special case in here, how would we feel about making 
`Type::isOverloadableType()` smarter, to return `false` for dependent types 
that can't possibly be class or enum types (eg, dependent pointer types, array 
types, and maybe more exotic things like function types, atomic types, and 
complex types)? This would then apply to all operators, not only unary `*`. Eg, 
we know the type of `` where `p` is `T*`, and we know the type of `p + n` 
where  `p` is `T*` and  `n` is integral. That change might have a lot more 
fallout, but it'd certainly be interesting to see what it breaks.

Is this change observable in some way? We should include a test with this 
change that demonstrates what effect it has. (The existing test in this patch 
passes with or without the change.) If nothing else I think we should be able 
to see the effect of this change in the output of `-ast-dump`. (Making the more 
general change I mentioned above may help here, by giving earlier diagnostics 
for some cases for which instantiation could never succeed, such as rejecting 
`p + q` where `p` and `q` are both pointer types.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D112453#3084394 , @courbet wrote:

> As per the comment in BuiltinTypes.def (see below), `Dependent` is
> allowed in context where the type is deducible, but is there any reason
> **not** to deduce the type if we can do it cheaply in some cases ?
>
>   // This represents the type of an expression whose type is
>   // totally unknown, e.g. 'T::foo'.  It is permitted for this to
>   // appear in situations where the structure of the type is
>   // theoretically deducible.
>   BUILTIN_TYPE(Dependent, DependentTy)

I've been trying to think if this will cause problems or not, and I'm not 
convinced one way or the other. I was thinking that if we resolve the type to a 
non-dependent type, but it is used within another type (making the second type 
also dependent), won't we change the point of instantiation for that second 
dependent type?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Ping ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

As per the comment in BuiltinTypes.def (see below), `Dependent` is
allowed in context where the type is deducible, but is there any reason
**not** to deduce the type if we can do it cheaply in some cases ?

  // This represents the type of an expression whose type is
  // totally unknown, e.g. 'T::foo'.  It is permitted for this to
  // appear in situations where the structure of the type is
  // theoretically deducible.
  BUILTIN_TYPE(Dependent, DependentTy)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112453/new/

https://reviews.llvm.org/D112453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
courbet added reviewers: rsmith, aaron.ballman.
courbet requested review of this revision.
Herald added a project: clang.

Example:

  template  auto f(T* t) {
return *t;
  }

Before that change, the `UnaryOperator` for `*t` has type ``.
After the change, its type is a `T` lvalue.

I've added simple tests to verify that giving knowledge of the type to clang
does not yields false positives in contexts where c++ does not allow it to use
that knowledge.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112453

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/dependent-type-identity.cpp


Index: clang/test/SemaTemplate/dependent-type-identity.cpp
===
--- clang/test/SemaTemplate/dependent-type-identity.cpp
+++ clang/test/SemaTemplate/dependent-type-identity.cpp
@@ -69,6 +69,16 @@
   void f8(typename N::X2::template apply *);
   void f8(typename N::X2::template apply *);
   void f8(typename ::Nalias::X2::template apply *); // 
expected-error{{redeclar}}
+
+  // (17.4.2): If an expression e is type-dependent (17.6.2.2), decltype(e)
+  // denotes a unique dependent type. Two such decltype-specifiers refer to the
+  // same type only if their expressions are equivalent (17.5.6.1)
+  T* a;
+  T* b;
+  using V = decltype(*a);
+  void f9(decltype(*a)); // expected-note{{previous}}
+  void f9(decltype(*b));
+  void f9(V); // expected-error{{redeclar}}
 };
 
 namespace PR6851 {
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -13323,10 +13323,22 @@
   ArrayRef ArgsArray(Args, NumArgs);
 
   if (Input->isTypeDependent()) {
-if (Fns.empty())
-  return UnaryOperator::Create(Context, Input, Opc, Context.DependentTy,
-   VK_PRValue, OK_Ordinary, OpLoc, false,
+if (Fns.empty()) {
+  QualType ResultTy = Context.DependentTy;
+  ExprValueKind ResultKind = VK_PRValue;
+  if (getLangOpts().CPlusPlus && (Opc == UO_Deref) &&
+  Input->getType()->isPointerType()) {
+// In c++, a deref of a `T*` always has type `T&` (16.6.8). There is no
+// way for other overloads to be selected since overloads of 
`operator*`
+// always have class or enum parameters.
+ResultTy = cast(Input->getType().getCanonicalType())
+   ->getPointeeType();
+ResultKind = VK_LValue;
+  }
+  return UnaryOperator::Create(Context, Input, Opc, ResultTy, ResultKind,
+   OK_Ordinary, OpLoc, false,
CurFPFeatureOverrides());
+}
 
 CXXRecordDecl *NamingClass = nullptr; // lookup ignores member operators
 ExprResult Fn = CreateUnresolvedLookupExpr(


Index: clang/test/SemaTemplate/dependent-type-identity.cpp
===
--- clang/test/SemaTemplate/dependent-type-identity.cpp
+++ clang/test/SemaTemplate/dependent-type-identity.cpp
@@ -69,6 +69,16 @@
   void f8(typename N::X2::template apply *);
   void f8(typename N::X2::template apply *);
   void f8(typename ::Nalias::X2::template apply *); // expected-error{{redeclar}}
+
+  // (17.4.2): If an expression e is type-dependent (17.6.2.2), decltype(e)
+  // denotes a unique dependent type. Two such decltype-specifiers refer to the
+  // same type only if their expressions are equivalent (17.5.6.1)
+  T* a;
+  T* b;
+  using V = decltype(*a);
+  void f9(decltype(*a)); // expected-note{{previous}}
+  void f9(decltype(*b));
+  void f9(V); // expected-error{{redeclar}}
 };
 
 namespace PR6851 {
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -13323,10 +13323,22 @@
   ArrayRef ArgsArray(Args, NumArgs);
 
   if (Input->isTypeDependent()) {
-if (Fns.empty())
-  return UnaryOperator::Create(Context, Input, Opc, Context.DependentTy,
-   VK_PRValue, OK_Ordinary, OpLoc, false,
+if (Fns.empty()) {
+  QualType ResultTy = Context.DependentTy;
+  ExprValueKind ResultKind = VK_PRValue;
+  if (getLangOpts().CPlusPlus && (Opc == UO_Deref) &&
+  Input->getType()->isPointerType()) {
+// In c++, a deref of a `T*` always has type `T&` (16.6.8). There is no
+// way for other overloads to be selected since overloads of `operator*`
+// always have class or enum parameters.
+ResultTy = cast(Input->getType().getCanonicalType())
+   ->getPointeeType();
+ResultKind = VK_LValue;
+  }
+  return UnaryOperator::Create(Context, Input, Opc, ResultTy, ResultKind,
+   OK_Ordinary, OpLoc, false,