Page MenuHomePhabricator

[clang-tidy] Add check performance-faster-string-find
ClosedPublic

Authored by sbenza on Jan 13 2016, 1:00 PM.

Details

Summary

Add check performance-faster-string-find.
It replaces single character string literals to character literals in calls to string::find and friends.

Diff Detail

Repository
rL LLVM

Event Timeline

sbenza updated this revision to Diff 44780.Jan 13 2016, 1:00 PM
sbenza retitled this revision from to [clang-tidy] Add check performance-faster-string-find.
sbenza updated this object.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.
alexfh edited edge metadata.Jan 14 2016, 4:47 AM

Awesome! See a few comments inline.

clang-tidy/performance/FasterStringFindCheck.cpp
51 ↗(On Diff #44780)

Probably out of scope of this patch, but I wonder how many times hasName is still more effective than one matchesName? Maybe we should add a matchesName variant for unqualified names or hasName taking a list of strings?

57 ↗(On Diff #44780)

const auto & would be slightly clearer.

75 ↗(On Diff #44780)

The message might be confusing in some situations (e.g. macros). I think, it should at least mention what method (and maybe of what class) is in question, e.g. "std::string::find() called with a string literal consisting of a single character; consider using the more effective overload accepting a character".

clang-tidy/performance/FasterStringFindCheck.h
15 ↗(On Diff #44780)

Sort includes, please.

test/clang-tidy/performance-faster-string-find.cpp
8 ↗(On Diff #44780)

Should we move stubs to a common header(s)?

9 ↗(On Diff #44780)

Did you mean const T *?

31 ↗(On Diff #44780)

As usual, please add tests with templates and macros ;)

32 ↗(On Diff #44780)

Please add tests for std::wstring, std::u16string and std::u32string. At the very least, we should ensure we don't break the code using them.

50 ↗(On Diff #44780)

"Some other methods." maybe?

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/performance/FasterStringFindCheck.cpp
25 ↗(On Diff #44780)

It might be nice for this to be more tolerant of whitespace around the commas.

47 ↗(On Diff #44780)

Can you also disable registration of these matches outside of C++ mode?

alexfh added inline comments.Jan 14 2016, 7:19 AM
clang-tidy/performance/FasterStringFindCheck.cpp
25 ↗(On Diff #44780)

I don't see much value in this, since it's not someone will change frequently (I guess, once per codebase on average).

aaron.ballman added inline comments.Jan 14 2016, 7:27 AM
clang-tidy/performance/FasterStringFindCheck.cpp
25 ↗(On Diff #44780)

I don't see much value in this, since it's not someone will change frequently (I guess, once per codebase on average).

Anything that's user-facing should be fault tolerant and sanitized for a better user experience.

alexfh added inline comments.Jan 14 2016, 7:47 AM
clang-tidy/performance/FasterStringFindCheck.cpp
25 ↗(On Diff #44780)

I also don't feel strongly about this. Trimming all strings after splitting should be a trivial 2-line addition to this patch.

sbenza updated this revision to Diff 44886.Jan 14 2016, 8:11 AM
sbenza marked 4 inline comments as done.
sbenza edited edge metadata.

Added template and macro tests.
Fixed warning message.

sbenza marked an inline comment as done.Jan 14 2016, 8:11 AM
sbenza added inline comments.
clang-tidy/performance/FasterStringFindCheck.cpp
51 ↗(On Diff #44780)

I have no idea how much more expensive is the regex engine.
It also makes the code harder to read.
I would totally be in favor of a variadic hasAnyName(). It is shorter and can be much faster to run.

75 ↗(On Diff #44780)

Used unqualified function name instead.
The qualified name for find is std::basic_string<blah, blah, blah>::find, which might be distracting.

sbenza updated this revision to Diff 44889.Jan 14 2016, 8:39 AM
sbenza marked 2 inline comments as done.

Added support for non 'char' chars.

sbenza marked 2 inline comments as done.Jan 14 2016, 8:39 AM
aaron.ballman added inline comments.Jan 14 2016, 9:38 AM
clang-tidy/performance/FasterStringFindCheck.cpp
29 ↗(On Diff #44889)

I think hasName() will assert if given an empty string, so this should probably also guard against a class list like ",basic_string".

test/clang-tidy/performance-faster-string-find.cpp
9 ↗(On Diff #44889)

Should we move stubs to a common header(s)?

Yes, please. However, that can be a separate patch that does something more comprehensive.

sbenza updated this revision to Diff 44901.Jan 14 2016, 10:28 AM
sbenza marked an inline comment as done.

More checks in argument parsing.

clang-tidy/performance/FasterStringFindCheck.cpp
29 ↗(On Diff #44889)

Also changed the separator to be ';' instead of ','.
The latter could be part of a type name. Eg. Foo<A,B>::Bar

aaron.ballman added inline comments.Jan 14 2016, 12:07 PM
clang-tidy/performance/FasterStringFindCheck.cpp
30 ↗(On Diff #44901)

Also changed the separator to be ';' instead of ','.
The latter could be part of a type name. Eg. Foo<A,B>::Bar

That's a great catch!

@alexfh -- do you think we should try to get our checkers to be consistent with their choice of list separators? Specifically, I am thinking about D16113. It seems like it would improve the user experience to not have to wonder "do I use comma, or semi colon, for this list?"

hokein added a subscriber: hokein.Jan 22 2016, 9:52 AM
hokein added inline comments.
clang-tidy/performance/FasterStringFindCheck.h
25 ↗(On Diff #44901)

I think you need to add document about StringLikeClasses option here.

alexfh added inline comments.Jan 22 2016, 2:55 PM
clang-tidy/performance/FasterStringFindCheck.cpp
30 ↗(On Diff #44901)

We can try to be consistent with the choice of delimiters, but I'm not sure we can use the same delimiter always without introducing more syntax complexities like escaping or the use of quotes (like in CSV). In any case, we should clearly document the format for each option and think how we can issue diagnostics when we suspect incorrect format used in the option value.

72 ↗(On Diff #44901)

This has to use the same delimiter. Maybe pull it to a constant to avoid inconsistency?

clang-tidy/performance/FasterStringFindCheck.h
25 ↗(On Diff #44901)

Not here, in the .rst file, please.

aaron.ballman added inline comments.Jan 25 2016, 7:33 AM
clang-tidy/performance/FasterStringFindCheck.cpp
30 ↗(On Diff #44901)

I think we should strive for consistency with ";" for right now, and agree that documentation/assistance is a reasonable fallback.

sbenza updated this revision to Diff 46796.Feb 3 2016, 8:42 AM

Added comment about StringLikeClasses

sbenza updated this revision to Diff 46798.Feb 3 2016, 8:59 AM

Make the delimiter a constant and fix mismatch between parse/serialize of the option.

Friendly ping.

aaron.ballman accepted this revision.Feb 12 2016, 7:38 AM
aaron.ballman edited edge metadata.

LGTM with one tiny nit.

clang-tidy/performance/FasterStringFindCheck.cpp
44 ↗(On Diff #46798)

Minor nit: missing punctuation for the comment.

This revision is now accepted and ready to land.Feb 12 2016, 7:38 AM
sbenza updated this revision to Diff 47808.Feb 12 2016, 8:30 AM
sbenza marked an inline comment as done.
sbenza edited edge metadata.

Minor fix on comment

This revision was automatically updated to reflect the committed changes.