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