[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

(Just added some side remarks about string handling annoyances.)

@balazske Please (1) handle the `dyn_cast` request of  @steakhal and also (2) do something with this "how to convert `StringRef` to `char *`" question that we're bikeshedding. I hope that after those we could finally merge this commit sequence.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305
+      std::string Note =
+          llvm::formatv(Case.getNote().str().c_str(),
+                        cast<NamedDecl>(Call.getDecl())->getDeclName());
----------------
steakhal wrote:
> donat.nagy wrote:
> > Consider using the method `StringRef::data()` which directly converts a `StringRef` to a `const char *`. Your two-step conversion has the advantage that it adds a `\0` terminator even if the `StringRef` isn't null-terminated, but I cannot imagine a "natural" code change that would introduce references to non-null-terminated char arrays as note message templates.
> I would prefer not to rely on that `StringRef`s (here) are expected to be null-terminated, unless benchmarks demonstrate that this is important. If that would turn out to be the case, then we would need to enforce this using some sort of `assert` expression.
I think the way of expressing this concrete conversion is a very low priority question (a.k.a. bikeshedding), so returning to the previously used two-step conversion is also fine for me.

The primary goal of this ".data()" shortcut was not performance, but clarity: those chained conversions triggered my "detect suspicious code" instincts and this was the best alternative that I could find. In general I feel that the LLVM codebase has way too many string types, and the back-and-forth conversions between them add lots of unnecessary noise to the code.

In this particular case I think it's the fault of `llvm::formatv` that it doesn't accept `StringRef` (the most commonly used non-owning string) and in general it's problematic design that there is no "good" conversion between `StringRef` and C-style strings in either direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612