Add code comment about the choice of hasAnyArgument

Nits

In D114823#3229990, @njames93 wrote:

I'm in 2 minds about this. This diagnostic is a good fit for this warning and shouldn't be removed. Likewise duplicate diagnostics from different checks is annoying. Maybe a better path forward would be to suppress the diagnostic if the bugprone-stringview-nullptr check is enabled. These kinds of intercheck dependencies are found in modernize-prefer-member-init check IIRC. You can just copy the impl from there.

Only filter out basic_string_view when bugprone-stringview-nullptr is enabled

Nit

Rebase on head

Nit

Switching back to original plan of adding support for return statements

Switch to using empty string edits everywhere except equality comparison

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

Delete non-functional test cases (which were previously commented out)

Add missing comment lines

Switch to early return for the filter

Pinging this thread with https://reviews.llvm.org/D113148

Rebase on head

Address nits

Thank you all for the feedback! Would it be ok to proceed with removing the nullptr checking for basic_string_view in bugprone-string-constructor, as Alex indicated?

Rebase on head

Address reviewer comments

Address review comments from Yitzie

Add missing test cases

Add additional tests for sv.empty() cases to ensure parens are removed where appropriate

Rebase on head

Add missing newline

Add AST matcher support and tests for {{}} case where a null pointer is implicitly created

Update test file to use truncated messages

Fix documentation nit

Address nits and update the absl::Cleanup stub to be accurate with the real interface (with associated changes to the AST matcher)

Thanks for the review! I'll apply the same fixes to the string_view diff as well

nits

Remove double brackets for list initialization and use default initialization to avoid most vexing parse

Change static_cast suggested edit to use parens instead of braces

Fix spelling error

Thanks for the additional info, @aaron.ballman! I had not considered the issues with duplicate warnings. I agree that it would be annoying for users. That being the case, the second option ("remove the string_view nullptr checking functionality from bugprone-string-constructor so it only lives in the new check") sounds the most appealing to me, personally.

In D113148#3108993, @aaron.ballman wrote:

Generally speaking, we prefer to improve the existing checks. I think bugprone-string-constructor would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the string_view-with-nullptr logic in one place. That said, we should be careful we're not too onerous when users enable all bugprone checks together.

Remove trailing whitespace

Add trailing newline

Remove unwanted changes

In D113148#3107897, @Sockke wrote:

This seems to be an existing check. Have you compared it with bugprone-string-constructor?

Add missing newline

Remove mistaken changes

Remove mistaken changes

Rebase on head

Update docs to reflect current state

Now with code! Will it run the tests?

Amended commit??

Trying again but with a branch to see if that helps

Test update message, please ignore

Test top-level comment, please ignore

In D17993#2398337, @jdoerfert wrote:

Please modify the commit subject and add a proper message.

In D17993#2367447, @rsmith wrote:

Thanks! We should also have a test for the behavior when targeting the MS ABI, where we sometimes don't emit the nonnull dereferenceable because the "this" pointer might actually point outside the object, but otherwise I think this is ready to go.

Please can you also put together a patch for the release notes? This seems worth mentioning there.

The new tests can be found in this-nonnull.cpp: https://reviews.llvm.org/differential/changeset/?ref=2242268

Apply dereferenceable even if null is a valid address