[PATCH] D22392: [Sema] Fix an invalid nullability warning for binary conditional operators

[Akira Hatanaka via cfe-commits]

ahatanak added inline comments.

================
Comment at: lib/Sema/SemaExpr.cpp:7007
@@ +7006,3 @@
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+                                  ASTContext &Ctx) {
----------------
doug.gregor wrote:
> This name could be improved. You're not really 'modifying' nullability here; in the general case, you're computing the appropriate nullability given the LHS, RHS, and applying that to the result type.
I'm thinking about renaming it to computeConditionalNullability or something, but I'm open to suggestions.

================
Comment at: lib/Sema/SemaExpr.cpp:7024
@@ +7023,3 @@
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
----------------
doug.gregor wrote:
> I'm fairly certain that more extensive testing will show that you need to have the LHSExpr as well, to look at the nullability of both.
This patch only tries to correct the case where you have a binary (shorthand version) conditional operator "p0 ?: p1" where p1 is nonnulll and p0 is nullable, in which case the result type should be nonnull. I think this function behaves correctly in this particular case, but it's possible that I have overlooked some corner cases.

In any case, I think the focus of this patch was too narrow. I think I should fix the nullability of any conditional operators, not just the shorthand versions, in my next patch.

================
Comment at: lib/Sema/SemaExpr.cpp:7030
@@ +7029,3 @@
+  // Create a new AttributedType with the new nullability kind.
+  QualType NewTy = ResTy.getDesugaredType(Ctx);
+  auto NewAttr = AttributedType::getNullabilityAttrKind(NewKind);
----------------
doug.gregor wrote:
>  It would be better to only unwrap sugar until we hit the nullability-attributed type, then replace it.
I think we need to unwrap sugar more than once until there are no more nullability-attributed types in some cases. For example, it's possible to have multiple levels of typedefs having nullability markers:

```
typedef int * IntP;
typedef IntP _Nullable NullableIntP0;
typedef NullableIntP0 _Nullable NullableIntP1;
```

================
Comment at: test/Sema/nullability.c:137
@@ +136,3 @@
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
----------------
doug.gregor wrote:
> You really need much more testing coverage here, e.g., for ternary operators where the types on the second and third arguments are different types (say, superclass/subclass pointer), the nullability is on either argument, etc. The ternary operator, especially in C++, has a ton of different cases that you need to look at.
Yes, I'll have more test cases in my next patch.


https://reviews.llvm.org/D22392