Page MenuHomePhabricator

Add -Wnullability-completeness-on-arrays.
ClosedPublic

Authored by jordan_rose on Oct 28 2016, 5:44 PM.

Details

Reviewers
doug.gregor
Summary

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 D25850. Part of rdar://problem/25846421.

Diff Detail

Repository
rL LLVM

Event Timeline

jordan_rose retitled this revision from to Add -Wnullability-completeness-on-arrays..
jordan_rose updated this object.
jordan_rose added a reviewer: doug.gregor.
jordan_rose set the repository for this revision to rL LLVM.
jordan_rose added a subscriber: cfe-commits.

Hi Jordan,

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

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

doug.gregor accepted this revision.Nov 9 2016, 1:07 PM
doug.gregor edited edge metadata.

Looks good! I appreciate the refactoring of recordNullabilitySeen

This revision is now accepted and ready to land.Nov 9 2016, 1:07 PM

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

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.

lib/Sema/SemaType.cpp
3988

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?

jordan_rose added inline comments.Nov 9 2016, 5:21 PM
lib/Sema/SemaType.cpp
3988

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.

ahatanak added inline comments.Nov 9 2016, 7:00 PM
lib/Sema/SemaType.cpp
3988

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.

jordan_rose closed this revision.Nov 10 2016, 4:49 PM

Committed as rL286520, with a slight fix-up for MSVC in rL286525.

rsanchezsaez added a subscriber: rsanchezsaez.EditedMar 28 2017, 10:32 AM

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?

jordan_rose added a comment.EditedMar 28 2017, 10:33 AM

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.

rsanchezsaez added a comment.EditedMar 28 2017, 10:35 AM

Oh, I see. Thanks for the explanation.

The fix-it syntax got me confused:

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

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!)

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