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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

This looks really good. An impressive effort! Just a few changes.  Also, please ping Alex to get his opinion on the high level issue.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:62
+          hasSourceExpression(StringViewConstructingFromNullExpr)),
+      changeTo(node("null_argument_expr"), cat("")), construction_warning);
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:67
+                             unless(hasParent(compoundLiteralExpr()))),
+      changeTo(node("null_argument_expr"), cat("")), construction_warning);
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:75
+      compoundLiteralExpr(has(StringViewConstructingFromNullExpr)),
+      changeTo(node("null_argument_expr"), cat("")), construction_warning);
+
----------------
And more below...


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:90
+      declStmt(
+          has(varDecl(has(StringViewConstructingFromNullExpr),
+                      unless(has(cxxConstructExpr(isListInitialization()))))
----------------
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).


================
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 */;
----------------
what are these lines doing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148