This is an archive of the discontinued LLVM Phabricator instance.

Filter string_view from the nullptr diagnosis of bugprone-string-constructor to prevent duplicate warnings with bugprone-stringview-nullptr
ClosedPublic

Authored by CJ-Johnson on Nov 30 2021, 3:18 PM.

Details

Summary

Updates the check and tests to not diagnose the null case for string_view (but retains it for string). This prevents the check from giving duplicate warnings that are caught by bugprone-stringview-nullptr (D113148).

Diff Detail

Event Timeline

CJ-Johnson created this revision.Nov 30 2021, 3:18 PM
CJ-Johnson requested review of this revision.Nov 30 2021, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 3:18 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

CJ -- please mention "bugprone-stringview-nullptr" explicitly and explain that it will cover those features, etc. so that the description will be clear on its own. Thanks!

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
178–179

Slightly prefer an early return here:
Filter out basic_string_view to avoid conflicts with
bugprone-stringview-nullptr
if (Result.Nodes.getNodeAs<CXXRecordDecl>("basic_string_view_decl") != nullptr)

return;
CJ-Johnson updated this revision to Diff 391040.Dec 1 2021, 8:33 AM
CJ-Johnson marked an inline comment as done.

Switch to early return for the filter

CJ-Johnson edited the summary of this revision. (Show Details)Dec 1 2021, 8:35 AM

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

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.

Great suggestion! I've updated this diff to do exactly that. Thanks! :)

ymandel accepted this revision.Jan 12 2022, 1:39 PM
This revision is now accepted and ready to land.Jan 12 2022, 1:39 PM