Page MenuHomePhabricator

Accept nullability annotations (_Nullable) on array parameters
ClosedPublic

Authored by jordan_rose on Oct 20 2016, 5:09 PM.

Details

Summary

Last year Apple added new qualifiers to pointer types: _Nullable, _Nonnull, and _Null_unspecified. This patch extends that to array types used in function declarations, which should have always been supported since they immediately decay to pointers.

void test(int ints[_Nonnull],
          void *ptrs[_Nullable],
          void **nestedPtrs[_Nullable],
          void ** _Nullable reference);

Follow-up work:

  • Figure out what to do with -Wnullability-completeness
  • Figure out how this affects #pragma clang assume_nonnull. Most likely the best answer is a new warning.

Part of rdar://problem/25846421

Diff Detail

Repository
rL LLVM

Event Timeline

jordan_rose retitled this revision from to [WIP] Accept nullability annotations (_Nullable) on array parameters.
jordan_rose updated this object.
jordan_rose added reviewers: aprantl, doug.gregor.
jordan_rose set the repository for this revision to rL LLVM.
jordan_rose added a subscriber: cfe-commits.
jordan_rose updated this object.Oct 20 2016, 5:13 PM
rsmith added a subscriber: rsmith.Oct 20 2016, 8:21 PM

This generally makes sense to me. _Nonnull in this position seems very similar to static (which typically also implies non-nullness).

lib/CodeGen/CGDebugInfo.cpp
2493–2499

I think this should be handled by UnwrapTypeForDebugInfo instead of here; this is after all just a type sugar node.

lib/Sema/SemaType.cpp
6659

I'm not sure this is enough; you should make sure you also reject

void f(int [5][_Nonnull 1]) {}

and the like.

jordan_rose added inline comments.Oct 25 2016, 11:41 AM
lib/CodeGen/CGDebugInfo.cpp
2493–2499

Getting back to this today. I'm inclined to say we should just drop the AdjustedType node altogether in UnwrapTypeForDebugInfo. What do you think? (also for @aprantl)

aprantl added inline comments.Oct 25 2016, 1:14 PM
lib/CodeGen/CGDebugInfo.cpp
2493–2499

Assuming that we don't want to support nullability attributes in the debug info that seems fine. At least at this point I don;t think there is a need to encode this in DWARF.

_Nonnull in this position seems very similar to static (which typically also implies non-nullness).

I wasn't actually sure if it was okay to assume this, but the standard does seem pretty clear:

If the keyword static also appears within the [ and ] of the array type derivation, then for each call to the function, the value of the corresponding actual argument shall provide access to the first element of an array with at least as many elements as specified by the size expression. (C11 6.7.6.3p7)

We can pick this up on the Swift side.

jordan_rose retitled this revision from [WIP] Accept nullability annotations (_Nullable) on array parameters to Accept nullability annotations (_Nullable) on array parameters.
jordan_rose updated this object.

Updated based on feedback from Richard, fixed typedef support, added tests.

Are there additional tests I should add?

jordan_rose marked an inline comment as done.Oct 25 2016, 5:36 PM

Oops. Ignore the API notes file, which is only in Swift's branch of Clang right now.

doug.gregor accepted this revision.Nov 9 2016, 11:49 AM
doug.gregor edited edge metadata.

This is looking great, Jordan.

This revision is now accepted and ready to land.Nov 9 2016, 11:49 AM
jordan_rose closed this revision.Nov 10 2016, 4:48 PM

Committed as rL286519.