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

[Doug Gregor via cfe-commits]

doug.gregor added a comment.

A bunch of comments above. This needs much more extensive testing, because there are numerous paths through the ternary operator code and the results need to be symmetric.


================
Comment at: lib/Sema/SemaExpr.cpp:7007
@@ +7006,3 @@
+/// expression.
+static QualType modifyNullability(QualType ResTy, Expr *RHSExpr,
+                                  ASTContext &Ctx) {
----------------
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.

================
Comment at: lib/Sema/SemaExpr.cpp:7024
@@ +7023,3 @@
+
+  NullabilityKind NewKind = GetNullability(RHSExpr->getType());
+
----------------
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.

================
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);
----------------
 It would be better to only unwrap sugar until we hit the nullability-attributed type, then replace it.

================
Comment at: test/Sema/nullability.c:137
@@ +136,3 @@
+
+  int * _Nonnull p2 = p0 ?: p1; // no warnings here.
+}
----------------
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.


https://reviews.llvm.org/D22392