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