[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Ricardo Sanchez-Saez via Phabricator via cfe-commits]

rsanchezsaez added a comment.

Haha! That makes sense. (I miss GitHub's comment reactions on Phabricator, 
consider your comments 'ed.)


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Yep, that's the correct syntax. It's not wonderful, but it's already where the 
C standard lets you put `const`, so we're just following established practice. 
(Learn something new and awful about C every day!)


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Ricardo Sanchez-Saez via Phabricator via cfe-commits]

rsanchezsaez added a comment.

Oh, I see. Thanks for the explanation.

The fix-it syntax got me confused:

  - (instancetype)initWithValues:(const int32_t [__null_unspecified])values


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

No, the *arrays themselves* need to be marked as nullable or non-nullable. 
Remember that in C array parameters are passed as pointers.

The compiler should give you a fix-it that shows the correct syntax.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Ricardo Sanchez-Saez via Phabricator via cfe-commits]

rsanchezsaez added a comment.

I think this makes the warning to trigger on methods such as:

  - (instancetype)initWithValues:(const int32_t [])values
  - (void)updateWithRange:(NSRange)range floatArray:(CGFloat[])array;

(experienced on the just released //Xcode 8.3//).

Perhaps I'm misunderstanding something, but shouldn't the warning only apply to 
arrays of pointer types?


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Committed as https://reviews.llvm.org/rL286520, with a slight fix-up for MSVC 
in https://reviews.llvm.org/rL286525.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Akira Hatanaka via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Sema/SemaType.cpp:3988
   // Allow arrays of auto if we are a generic lambda parameter.
   // i.e. [](auto ()[5]) { return array[0]; }; OK
   if (AT && D.getContext() != Declarator::LambdaExprParameterContext) {

jordan_rose wrote:
> ahatanak wrote:
> > This isn't something that is related to the changes made in this patch, but 
> > it looks like we issue a -Wnullability-completeness warning when there is a 
> > generic lambda like this:
> > 
> > ```
> > auto G2 = [](auto a){};
> > ```
> > 
> > Is this expected?
> It's not desired, but it's not a regression either. D26226 mentions how this 
> comes up with explicit templates, so it doesn't surprise me that it happens 
> with generic lambdas too.
It turns out it has nothing to do with this patch or your other patches. The 
warning is issued when there is a variable initialized with any kind of lambda:

```
auto G2 = [](int a){};
```

This happens because Type::canHaveNullability() returns true when it is an 
undeduced auto and checkNullabilityConsistency records its location.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/Sema/SemaType.cpp:3988
   // Allow arrays of auto if we are a generic lambda parameter.
   // i.e. [](auto ()[5]) { return array[0]; }; OK
   if (AT && D.getContext() != Declarator::LambdaExprParameterContext) {

ahatanak wrote:
> This isn't something that is related to the changes made in this patch, but 
> it looks like we issue a -Wnullability-completeness warning when there is a 
> generic lambda like this:
> 
> ```
> auto G2 = [](auto a){};
> ```
> 
> Is this expected?
It's not desired, but it's not a regression either. D26226 mentions how this 
comes up with explicit templates, so it doesn't surprise me that it happens 
with generic lambdas too.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

In https://reviews.llvm.org/D26108#591270, @jordan_rose wrote:

> Ah, my apologies, @ahatanak. I was testing against the Swift branch of Clang.


That's OK. I haven't tested it yet, but the patch itself looks fine to me.




Comment at: lib/Sema/SemaType.cpp:3988
   // Allow arrays of auto if we are a generic lambda parameter.
   // i.e. [](auto ()[5]) { return array[0]; }; OK
   if (AT && D.getContext() != Declarator::LambdaExprParameterContext) {

This isn't something that is related to the changes made in this patch, but it 
looks like we issue a -Wnullability-completeness warning when there is a 
generic lambda like this:

```
auto G2 = [](auto a){};
```

Is this expected?


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via cfe-commits]

jordan_rose added a comment.

Ah, my apologies, @ahatanak. I was testing against the Swift branch of Clang.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Doug Gregor via cfe-commits]

doug.gregor accepted this revision.
doug.gregor added a comment.
This revision is now accepted and ready to land.

Looks good! I appreciate the refactoring of recordNullabilitySeen


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via cfe-commits]

jordan_rose added a comment.

It works fine for me, though note the "depends on 
https://reviews.llvm.org/D25850;. The other patches in the series do seem to 
have been thrown off by https://reviews.llvm.org/D26226 landing first, though, 
so I'll update those.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Akira Hatanaka via cfe-commits]

ahatanak added a comment.

Hi Jordan,

It seems that this patch doesn't apply cleanly. Can you rebase it?


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via cfe-commits]

jordan_rose created this revision.
jordan_rose added a reviewer: doug.gregor.
jordan_rose added a subscriber: cfe-commits.
jordan_rose set the repository for this revision to rL LLVM.
jordan_rose added a dependency: D25850: Accept nullability annotations 
(_Nullable) on array parameters.

This is an addition to (and sub-warning of) -Wnullability-completeness that 
warns when an array parameter is missing nullability. When the specific warning 
is switched off, the compiler falls back to only warning on pointer types 
written as pointer types.

Note that use of nullability //within// an array triggers the completeness 
checks regardless of whether or not the array-specific warning is enabled; the 
intent there is simply to determine whether a particular header is trying to be 
nullability-aware at all.

Depends on https://reviews.llvm.org/D25850. Part of rdar://problem/25846421.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/SemaObjCXX/Inputs/nullability-consistency-arrays.h
  test/SemaObjCXX/nullability-consistency-arrays.mm

Index: test/SemaObjCXX/nullability-consistency-arrays.mm
===
--- /dev/null
+++ test/SemaObjCXX/nullability-consistency-arrays.mm
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -Wno-nullability-completeness -Werror
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -Wno-nullability-completeness -Werror
+
+#include "nullability-consistency-arrays.h"
+
+void h1(int *ptr) { } // don't warn
+
+void h2(int * _Nonnull p) { }
Index: test/SemaObjCXX/Inputs/nullability-consistency-arrays.h
===
--- /dev/null
+++ test/SemaObjCXX/Inputs/nullability-consistency-arrays.h
@@ -0,0 +1,87 @@
+void firstThingInTheFileThatNeedsNullabilityIsAnArray(int ints[]);
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+int *secondThingInTheFileThatNeedsNullabilityIsAPointer;
+#if !ARRAYS_CHECKED
+// expected-warning@-2 {{pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+int *_Nonnull triggerConsistencyWarnings;
+
+void test(
+int ints[],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+void *ptrs[], // expected-warning {{pointer is missing a nullability type specifier}}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+void **nestedPtrs[]); // expected-warning 2 {{pointer is missing a nullability type specifier}}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+void testArraysOK(
+int ints[_Nonnull],
+void *ptrs[_Nonnull], // expected-warning {{pointer is missing a nullability type specifier}}
+void **nestedPtrs[_Nonnull]); // expected-warning 2 {{pointer is missing a nullability type specifier}}
+void testAllOK(
+int ints[_Nonnull],
+void * _Nullable ptrs[_Nonnull],
+void * _Nullable * _Nullable nestedPtrs[_Nonnull]);
+
+void nestedArrays(int x[5][1]) {}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+void nestedArraysOK(int x[_Nonnull 5][1]) {}
+
+#if !__cplusplus
+void staticOK(int x[static 5][1]){}
+#endif
+
+int globalArraysDoNotNeedNullability[5];
+
+typedef int INTS[4];
+
+void typedefTest(
+INTS x,
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+INTS _Nonnull x2,
+_Nonnull INTS x3,
+INTS y[2],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+INTS y2[_Nonnull 2]);
+
+
+#pragma clang assume_nonnull begin
+void testAssumeNonnull(
+  int ints[],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}