[PATCH] D115121: Add support for return values in bugprone-stringview-nullptr

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:75
 
-  auto HandleTemporaryCXXStaticCastExpr = makeRule(
-      cxxStaticCastExpr(
-          hasSourceExpression(StringViewConstructingFromNullExpr)),
-      changeTo(node("null_argument_expr"), cat("\"\"")), construction_warning);
+  auto HandleTemporaryReturnValue = makeRule(
+      returnStmt(
----------------
In these cases, what is special about `return`? I'd guess the AST has various implicit nodes inserted, but then might it make  more sense to focus on those as the pattern? Or, is the edit different based on the return context? Might be worth explaining this more in a comment?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp:77
+      returnStmt(
+          hasReturnValue(ignoringImpCasts(StringViewConstructingFromNullExpr))),
+      changeTo(node("construct_expr"), cat("{}")), construction_warning);
----------------
Is it possible for this rule to overlap with any of the others, but not at the `returnStmt` level? Specifically, might the `ignoringImpCasts` overlap with any of the other expression patterns above? I think not, but am not quite sure where `cxxTemporaryObjectExpr` can show up. 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp:94
+
+// TODO: Handle cases where types, such as Class and Struct below, are
+//       constructed with null arguments.
----------------
CJ-Johnson wrote:
> I attempted to address this TODO but it is quite the rabbit hole. If I just add a basic fallback matcher, `applyFirst(...)` does not have the behavior I desire. It was stomping on other, better fixes.



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp:94-95
+
+// TODO: Handle cases where types, such as Class and Struct below, are
+//       constructed with null arguments.
+class Class {
----------------
ymandel wrote:
> CJ-Johnson wrote:
> > I attempted to address this TODO but it is quite the rabbit hole. If I just add a basic fallback matcher, `applyFirst(...)` does not have the behavior I desire. It was stomping on other, better fixes.
> 
I'm not sure I understand the code you're trying to catch. Is it constructions of `Class` and `Struct`? If so, can you show me some example code snippets/transformations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115121