[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