[PATCH] D154880: [-Wunsafe-buffer-usage][WIP] Add a facility for debugging low fixit coverage.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Awesome!!

Did you try running it on some real code? Does this actually cover most cases? (I suspect that (1.) is going to be the most popular case, but that's also the easiest case to diagnose visually. We might still want a note if we wanted to prioritize among non-variables, but that's some work, maybe we can get back to this later.)

I have a few minor nitpicks, but generally LGTM!



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1745-1746
+                                    const StringRef UserFillPlaceHolder,
+                                    UnsafeBufferUsageHandler &Handler) {
   const QualType &SpanEltT = D->getType()->getPointeeType();
   assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
----------------
We might want to silence `-Wunused-parameter` in release builds.

Maybe it's better to put the entire parameter under `#ifndef NDEBUG`, but it's definitely more clumsy.

There's also `__attribute__((unused))` aka `[[maybe_unused]]` but it looks like we're not supposed to use it in LLVM for variables:
```
  173 // Some compilers warn about unused functions. When a function is sometimes
  174 // used or not depending on build settings (e.g. a function only called from
  175 // within "assert"), this attribute can be used to suppress such warnings.
  176 //
  177 // However, it shouldn't be used for unused *variables*, as those have a much
  178 // more portable solution:
  179 //   (void)unused_var_name;
  180 // Prefer cast-to-void wherever it is sufficient.
  181 #if __has_attribute(unused)
  182 #define LLVM_ATTRIBUTE_UNUSED __attribute__((__unused__))
  183 #else
  184 #define LLVM_ATTRIBUTE_UNUSED
  185 #endif
```


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1760-1761
+#ifndef NDEBUG
+      // FIXME: F->getBaseStmt() should never be null!
+      // (Or we should build a better interface for this.)
+      Handler.addDebugNoteForVar(
----------------
This comment is probably duplicated everywhere by accident; it doesn't make much sense in most of these places.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1790
+          ("failed to produce fixit for declaration '" + D->getName()
+           + "' : fale dto get end char loc (macro)").str());
+#endif
----------------



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2395
+              it->first, it->first->getBeginLoc(),
+              ("'" + it->first->getName() +
+               "' is neither local nor a parameter").str());
----------------
`getName()` crashes on various anonymous variables, whereas `getNameAsString()` always gives an answer (even if it's not always a "good" answer), which is more suitable for diagnostics.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2396
+              ("'" + it->first->getName() +
+               "' is neither local nor a parameter").str());
+#endif
----------------
Do you want to include a "failde to produce fixit..." text in this message and the next message, for consistency?


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

https://reviews.llvm.org/D154880