[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[CJ Johnson via Phabricator via cfe-commits]

CJ-Johnson marked 5 inline comments as done.
CJ-Johnson added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:75
+      compoundLiteralExpr(has(StringViewConstructingFromNullExpr)),
+      changeTo(node("null_argument_expr"), cat("")), construction_warning);
+
----------------
ymandel wrote:
> And more below...
Updated all 9 of them. Thanks!


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:90
+      declStmt(
+          has(varDecl(has(StringViewConstructingFromNullExpr),
+                      unless(has(cxxConstructExpr(isListInitialization()))))
----------------
ymandel wrote:
> Here and throughout the matchers: in general, `has` is a fallback when you don't have a more specific matcher. In this case, It think you mean to be testing the initialization expression, in which case you should query that directly: `hasInitializer`.  I'd recommend you revisit the uses of `has` and check in each case if there's a more specific query. That's both cheaper (doesn't need to iterate through all children) and more readable (because it expresses the intent).
Thanks for the suggestion! I went back through all of the has calls and replaced the ones I could with something more specific.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp:134-147
+    // (void)(const std::string_view{nullptr}) /* a16 */;
+    // CV qualifiers do not compile in this context
+
+    // (void)(const std::string_view{(nullptr)}) /* a17 */;
+    // CV qualifiers do not compile in this context
+
+    // (void)(const std::string_view{{nullptr}}) /* a18 */;
----------------
ymandel wrote:
> what are these lines doing?
These lines are not "tests" themselves but they clearly document that all of the various permutations have been considered. If someone reads the test file and thinks "what about this other case?", these demonstrate that such other cases have been considered but are not valid :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148