This is an archive of the discontinued LLVM Phabricator instance.

Update bugprone-stringview-nullptr to support return statements and constructors for any T which accepts basic_string_view
ClosedPublic

Authored by CJ-Johnson on Dec 5 2021, 11:13 AM.

Details

Summary

This diff changes bugprone-stringview-nullptr in the following ways:

  • Adds support for return statements, with new tests
  • Adds support for arbitrary T constructor calls that have string_view parameter type, with new tests
  • Adds filtering in the heavily re-used BasicStringViewConstructingFromNullExpr matcher to prevent collision with CXXTemporaryObjectExpr
  • Switches the implementation of BasicStringViewConstructingFromNullExpr to use hasAnyArgument instead of hasArgument to avoid the automated "skip" that the latter performs on parens and implicit casts
  • Adds the isDirectInitialization matcher to distinguish direct and copy variable declarations
  • Constraints variable declarations (and similar) to ensure only basic_string_view initialization is matched
  • Simplifies matching on function call expressions by only editing a single parameter at a time. Re-running the tool and applying the edits each time will allow it to catch multiple arguments in the same function call, in the rare case that is needed.

Diff Detail

Event Timeline

CJ-Johnson created this revision.Dec 5 2021, 11:13 AM
CJ-Johnson requested review of this revision.Dec 5 2021, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2021, 11:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
CJ-Johnson added inline comments.Dec 5 2021, 11:15 AM
clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
273–296

The diff isn't great here, but these weren't actually changed. I just inserted the new one, HandleTemporaryReturnValue, and it caused a reflow.

CJ-Johnson updated this revision to Diff 391940.Dec 5 2021, 1:45 PM

Add missing tests for function arguments and fix incorrect warning message for static_cast cases

CJ-Johnson added inline comments.Dec 5 2021, 1:46 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
1037–1038

This diff is also noisy, but it's about adding missing test cases

CJ-Johnson edited the summary of this revision. (Show Details)Dec 5 2021, 1:48 PM
CJ-Johnson added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
133–134

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.

ymandel added inline comments.Dec 6 2021, 6:36 AM
clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
109–110

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?

111

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.

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
133
133–134

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?

CJ-Johnson updated this revision to Diff 392157.Dec 6 2021, 1:19 PM

Switch to using empty string edits everywhere except equality comparison

CJ-Johnson marked 4 inline comments as done.Dec 6 2021, 1:20 PM
CJ-Johnson retitled this revision from Add support for return values in bugprone-stringview-nullptr to Update bugprone-stringview-nullptr to prefer the empty string for most edits.Dec 7 2021, 9:22 AM
CJ-Johnson edited the summary of this revision. (Show Details)

Switching back to original plan of adding support for return statements

CJ-Johnson retitled this revision from Update bugprone-stringview-nullptr to prefer the empty string for most edits to Update bugprone-stringview-nullptr to support return statements and constructors for any T which accepts basic_string_view.Jan 11 2022, 5:12 PM
CJ-Johnson edited the summary of this revision. (Show Details)
CJ-Johnson edited the summary of this revision. (Show Details)

Nit

Rebase on head

ymandel accepted this revision.Jan 12 2022, 1:38 PM

Bravo! This is an impressively thorough job. Thanks for pushing through to completion!

clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
265

precede with argumentCountIs(1)? Also, please comment on choice of hasAnyArgument (copying what you wrote in the patch description is fine).

This revision is now accepted and ready to land.Jan 12 2022, 1:38 PM
CJ-Johnson marked an inline comment as done.

Add code comment about the choice of hasAnyArgument

clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
265

Added the code comments, but not the argument count matcher. This case intentionally matches any number of arguments since some types make take 2+ parameters where one of them is string_view.

ymandel added inline comments.Jan 12 2022, 1:52 PM
clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
265

got it. I misunderstood the existing comment. thx

This revision was landed with ongoing or failed builds.Jan 12 2022, 1:57 PM
This revision was automatically updated to reflect the committed changes.