[PATCH] D27837: Add fix-it notes to the nullability consistency warning

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8772
+def note_nullability_fix_it : Note<
+  "insert '%select{_Nonnull|_Nullable|_Null_unspecified}0' if the "
+  "%select{pointer|block pointer|member pointer|array parameter}1 "
----------------
ahatanak wrote:
> 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.
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.


================
Comment at: lib/Sema/SemaType.cpp:3491
+/// taking into account whitespace before and after.
+static FixItHint fixItNullability(Sema &S, SourceLocation pointerLoc,
+                                  NullabilityKind nullability) {
----------------
arphaman wrote:
> 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.
Fair enough, will change. Been working in Swift too much. :-)


================
Comment at: lib/Sema/SemaType.cpp:3495
+
+  SmallString<32> insertionTextBuf{" "};
+  insertionTextBuf += getNullabilitySpelling(nullability);
----------------
arphaman wrote:
> Another NIT: I think it's better to avoid the braced initializer here, and use `SmallString<32> insertionTextBuf = " "` instead
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27837