[PATCH] D143680: [WIP][-Wunsafe-buffer-usage] Improve fix-its for local variable declarations with null pointer initializers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Looks great! Nitpicks.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:872
+  assert(Loc.isValid() && "Expected the source location of the last"
+                          "character of an AST Node is alwasy valid");
+  return Loc;
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:886-887
+  //   `Lexer::getLocForEndOfToken`)
+  assert(Loc.isValid() && "Expected the source location just past the last "
+                          "character of an AST Node is alwasy valid");
+  return Loc;
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:950
+  if (Init->isNullPointerConstant(
+          std::remove_const_t<ASTContext &>(Ctx),
+          // FIXME: Why does this function not ask for `const ASTContext
----------------
It's a bit intrusive but probably nicer to simply remove `const` from all our `ASTContext &` parameters.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1034-1035
+    assert(!InitFixIts.empty() &&
+           "Expected at least one fix-it for an initializer of an unsafe "
+           "variable declaration.");
     // The loc right before the initializer:
----------------
The old check was saying "`populateInitializerFixItWithSpan()` returns `{}` on error". So the new check is additionally saying "`populateInitializerFixItWithSpan()` isn't allowed to have errors anymore", which is arguably the more important message 🤔


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143680/new/

https://reviews.llvm.org/D143680