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