This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] abseil-redundant-strcat-calls-check
ClosedPublic

Authored by hugoeg on Aug 22 2018, 3:25 PM.

Details

Summary

Flags redundant calls to `absl::StrCat when the result is being passed to another call of absl::StrCat/absl::StrAppend`. Also suggests a fix to collapse the calls.
Example:
StrCat(1, StrCat(2, 3)) ==> StrCat(1, 2, 3)

The check was developed internally and has been running at google, this is just
a move to open source the check. It was originally written by @sbenza and has been modified for external use.

Diff Detail

Event Timeline

hugoeg created this revision.Aug 22 2018, 3:25 PM

Is it possible to implement fix-it?

clang-tidy/abseil/RedundantStrcatCallsCheck.cpp
97

auto could be used because of iterator on container.

98

continue should be on own line. Please run Clang-format.

130

Return type is not obvious, so auto should not be used.

docs/ReleaseNotes.rst
76

Please remove This check

'' are used instead of ``.

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
7

Please synchronize with Release Notes.

11

Please remove leading space.

14

Please add examples.

Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.
Eugene.Zelenko added a subscriber: cfe-commits.
hugoeg marked 7 inline comments as done.Aug 23 2018, 6:34 AM

If the fix-it can be implemented, I certainly do not know how. As mentioned above, I am not the original author for this check.

hugoeg updated this revision to Diff 162174.Aug 23 2018, 6:40 AM

corrections applied

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
17

Please add namespaces and use empty line instead of ==>

hugoeg updated this revision to Diff 162219.Aug 23 2018, 10:19 AM
hugoeg marked an inline comment as done.
docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
17

std::string. Please insert empty line.

18

std::string. Same indentation as in previous line. Same for second example.

hugoeg updated this revision to Diff 162223.Aug 23 2018, 10:24 AM
hugoeg marked 2 inline comments as done.
hugoeg updated this revision to Diff 162256.Aug 23 2018, 1:34 PM

minor fixes and style improvement

hugoeg edited the summary of this revision. (Show Details)Aug 23 2018, 1:35 PM
aaron.ballman added inline comments.Aug 24 2018, 7:24 AM
clang-tidy/abseil/RedundantStrcatCallsCheck.cpp
29

The formatting here looks off -- you should run the patch through clang-format, if you haven't already.

38–39

Can this be replaced by anyOf(CallToStrcat, CallToStrappend)?

51

s/hints/Hints

97

const auto * please (so it's obvious the type is a pointer).

100

s/sub/Sub

114–120

Elide braces

137

This text doesn't really help the user to understand what they've done wrong with their code. Perhaps multiple calls to 'StrCat' can be flattened into a single call or something along those lines?

hugoeg updated this revision to Diff 162382.Aug 24 2018, 8:13 AM
hugoeg marked 7 inline comments as done.

applied corrections form comments

hugoeg added inline comments.Aug 24 2018, 8:42 AM
clang-tidy/abseil/RedundantStrcatCallsCheck.cpp
38–39

I just tried it, it doesn't work, so we should probably keep it as is.

hugoeg marked an inline comment as done.Aug 24 2018, 8:42 AM
aaron.ballman added inline comments.Aug 24 2018, 9:45 AM
clang-tidy/abseil/RedundantStrcatCallsCheck.cpp
29

This comment is marked done but the formatting in the patch still doesn't match the LLVM coding style. For instance, pointers bind to the right, not to the left in our style.

136

One more change, based on comments in other reviews, this should probably be 'absl::StrCat' instead of just 'StrCat'.

hugoeg updated this revision to Diff 162405.Aug 24 2018, 10:06 AM
hugoeg marked an inline comment as done.

minor updates

aaron.ballman accepted this revision.Aug 24 2018, 10:27 AM

Aside from the formatting and one small documentation nit, LGTM!

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
8

to another -> to another call to

This revision is now accepted and ready to land.Aug 24 2018, 10:27 AM
hugoeg updated this revision to Diff 162438.Aug 24 2018, 12:08 PM
hugoeg marked 2 inline comments as done.

added corrections

Example in documentation was not fixed.

docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
17

Will be good idea to add before/after comments. Indentation is not fixed and empty line was not inserted. Same or second example.

hugoeg updated this revision to Diff 162454.Aug 24 2018, 1:09 PM
hugoeg updated this revision to Diff 162455.
hugoeg marked an inline comment as done.

Indentation of example in documentation is still fixed.

hugoeg updated this revision to Diff 162462.Aug 24 2018, 1:22 PM

Indentation of example in documentation is still fixed.

yea sorry about that, I caught that after i updated the diff and corrected and re updated

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive. Thank you for your time!

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive. Thank you for your time!

When I try to apply your patch on top of ToT, I get merge conflicts. Can you resolve those and upload a new patch?

hugoeg updated this revision to Diff 162932.Aug 28 2018, 12:28 PM

merge conflicts resolved @aaron.ballman

aaron.ballman closed this revision.Aug 29 2018, 4:30 AM

I've commit in r340918. Thank you for the patch!