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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
