Page MenuHomePhabricator

[clang-tidy] Extend bugprone-string-constructor-check to std::string_view.
ClosedPublic

Authored by ckennelly on Nov 7 2020, 1:42 PM.

Details

Summary

This allows for matching the constructors std::string has in common with
std::string_view.

Diff Detail

Event Timeline

ckennelly created this revision.Nov 7 2020, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2020, 1:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ckennelly requested review of this revision.Nov 7 2020, 1:42 PM
lebedev.ri retitled this revision from Extend bugprone-string-constructor-check to std::string_view. to [clang-tidy] Extend bugprone-string-constructor-check to std::string_view..Nov 7 2020, 10:19 PM
lebedev.ri edited reviewers, added: aaron.ballman, njames93, JonasToth; removed: EricWF, ymandel.
aaron.ballman added inline comments.Nov 9 2020, 6:58 AM
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
58

You should fix the lint warning.

59

It took me a while to realize that this functionality is really about trying to get to the name of the constructor for a given type -- perhaps this should be a single function called getCtorNameFromType() or something?

Actually, given that this is a narrowing matcher from a CXXConstructExpr, can't you look at CXXConstructExpr::getConstructor() to get the CXXConstructorDecl and check the name from there? That seems cleaner than trying to identify the constructor name through string matching.

ymandel added inline comments.
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
29–33

hasAnyName is a VariadicMatcher, so it supports passing a list directly (as a single argument), see ASTMatchersInternal.h, line 103.

Given that, I'm pretty sure this function is unnecessary; you can just call hasAnyName directly below instead of this function.

103–104

Following up on Aaron's comment above, I think you want something like this: hasDeclaration(cxxConstructorDecl(ofClass(cxxRecordDecl(hasAnyName(removeNamespaces(StringNames))))

128

nit: I'd put this comment line before the anyOf since it describes the whole thing, while the following comments describe each branch, respectively.

ckennelly updated this revision to Diff 305649.Nov 16 2020, 8:45 PM

Applied review feedback

ckennelly updated this revision to Diff 305652.Nov 16 2020, 9:07 PM
ckennelly marked 5 inline comments as done.

Applied review comments

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
59

I applied ymandel's suggestion, which I think simplified things a bit.

LGTM. I'll leave it to Aaron, though, to accept.

This revision is now accepted and ready to land.Nov 18 2020, 7:57 AM
This revision was landed with ongoing or failed builds.Nov 18 2020, 6:16 PM
This revision was automatically updated to reflect the committed changes.