Page MenuHomePhabricator

Add fix-it notes to the nullability consistency warning
ClosedPublic

Authored by jordan_rose on Dec 15 2016, 6:02 PM.

Details

Summary

This is especially important for arrays, since no one knows the proper syntax for putting qualifiers in arrays.

nullability.h:3:26: warning: array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)
void arrayParameter(int x[]);
                         ^
nullability.h:3:26: note: insert '_Nullable' if the array parameter may be null
void arrayParameter(int x[]);
                         ^
                          _Nullable
nullability.h:3:26: note: insert '_Nonnull' if the array parameter should never be null
void arrayParameter(int x[]);
                         ^
                          _Nonnull

rdar://problem/29524992

Diff Detail

Repository
rL LLVM

Event Timeline

jordan_rose retitled this revision from to Add fix-it notes to the nullability consistency warning.
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.
arphaman added inline comments.
lib/Sema/SemaType.cpp
3491

NIT: I noticed that you don't follow LLVM's naming style for variables here (the parameter and variable names should start with an upper case letter). I realise that there are some functions around the new functions in this file that don't follow this style, so this might be better for local consistency, but it would be nice if the new code follows LLVM's style.

3495

Another NIT: I think it's better to avoid the braced initializer here, and use SmallString<32> insertionTextBuf = " " instead

ahatanak added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8772

Is the third option (_Null_unspecified) going to be used somewhere? I see the first two options are used in emitNullabilityConsistencyWarning, but not the third one.

jordan_rose added inline comments.Dec 16 2016, 8:53 AM
include/clang/Basic/DiagnosticSemaKinds.td
8772

I think it's better not to suggest it to people, but it seemed weirder to have a reusable diagnostic that's intended to take a NullabilityKind and then have it blow up if someone ever decided to use it with NullUnspecified. I can take it out if you like.

lib/Sema/SemaType.cpp
3491

Fair enough, will change. Been working in Swift too much. :-)

3495

I tried that first, but SmallString doesn't have a valid copy-constructor. If it really bugs you I could make it a += line too.

ahatanak added inline comments.Dec 16 2016, 11:39 AM
include/clang/Basic/DiagnosticSemaKinds.td
8772

It seems that the code in lib/Basic/Diagnostic.cpp would assert at runtime if you tried to pass a value that isn't in the expected range (0 or 1 if _Null_unspecified were removed). But I guess you can leave _Null_unspecified there if you think it makes more sense to do so.

Updated names to match LLVM style.

jordan_rose accepted this revision.Dec 19 2016, 1:12 PM
jordan_rose added a reviewer: jordan_rose.

Doug was okay with the idea and I trust Alex and Akira would have caught any major problems. Committed in r290132.

This revision is now accepted and ready to land.Dec 19 2016, 1:12 PM
jordan_rose closed this revision.Dec 19 2016, 1:12 PM