This is an archive of the discontinued LLVM Phabricator instance.

[Clang Tidy]: Use shrink_to_fit instead of copy and swap trick.
ClosedPublic

Authored by xazax.hun on Jan 21 2015, 3:34 AM.

Details

Reviewers
alexfh
Summary

The \c shrink_to_fit() method is more readable and more effective than
the copy and swap trick to reduce the capacity of a shrinkable container.
Note that, the \c shrink_to_fit() method is only available in C++11 and up.

Example:

std::vector<int>(v).swap(v); will be replaced with v.shrink_to_fit();

There was already a checker which targeted C++11, however it did not contain any check, which standard is used during the analysis. Maybe a check should be added to suppress C++11 specific warnings when clang tidy is invoked in C++03 mode?

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 18495.Jan 21 2015, 3:34 AM
xazax.hun retitled this revision from to [Clang Tidy]: Use shrink_to_fit instead of copy and swap trick..
xazax.hun updated this object.
xazax.hun edited the test plan for this revision. (Show Details)
xazax.hun added a reviewer: alexfh.
xazax.hun added a subscriber: Unknown Object (MLST).
alexfh edited edge metadata.Jan 22 2015, 5:12 AM

Thanks for contributing the check! The code looks mostly good, but see a few comments inline.

There was already a checker which targeted C++11, however it did not contain any check, which
standard is used during the analysis. Maybe a check should be added to suppress C++11 specific
warnings when clang tidy is invoked in C++03 mode?

We could bail out in the check() method it the language standard is below C++11. Ideally, standard-specific checks would be able to avoid registering matchers when parsing code not in supported standard mode. However, there's no good way to do this as we don't know which minimum standard version the code is supposed to be compatible with. And at the point where we register matchers, there's no information on the list of files and the used compilation options yet. So looking at the standard in the check() method is the best we can do.

clang-tidy/readability/ShrinkToFitCheck.cpp
49

Maybe also handle pointer-type variables/data members?

72

Please use Result.Context->getLangOpts() instead (I've fixed the other two checks using LangOptions()).

75

s/The/the/

80

?

clang-tidy/readability/ShrinkToFitCheck.h
19

nit: I don't know whether Doxygen cares, but from the human point of view, \c would make more sense next to the word it relates to.

test/clang-tidy/readability-shrink-to-fit.cpp
16

nit: Please put the "&" to the variable.

40

I don't know why the fix doesn't get applied here (which would be wrong). I think, the check is avoiding this by pure coincidence. But it may be fine to postpone solving this until someone stumbles upon the problem and has a good test case.

48

Please add

// CHECK-FIXES: {{^  }}COPY_AND_SWAP_INT_VEC(v);{{$}}

to ensure this doesn't get spoiled.

xazax.hun updated this revision to Diff 18615.Jan 22 2015, 6:50 AM
xazax.hun edited edge metadata.

Thank you for the review!

  • I have modified the doxygen documentation as you recommended.
  • The actual language options are used.
  • No warnings should be generated for files that compiled without C++11 support (although I am not sure that this method works for headers in general).
  • Fixed the warning string and removed the redundant empty statement.
  • Do not try to fix code that is the result of a macro expansion (I am not sure that my solution is idiomatic).
  • Added pointer support.
  • Fixed some style issues.
alexfh added inline comments.Jan 22 2015, 7:27 AM
clang-tidy/readability/ShrinkToFitCheck.cpp
52

nit: It looks like there's one pair of parentheses more than needed. Also below on lines 57-59.

test/clang-tidy/readability-shrink-to-fit.cpp
47

Can you add one more test?

in g():

...
T v2;
T(v2).swap(v2);

in h():

g<std::vector<int>>();
xazax.hun updated this revision to Diff 18667.Jan 23 2015, 5:43 AM

Thank you for pointing out the issue. It looks like not matching on instantiations solves the problem.

alexfh accepted this revision.Jan 23 2015, 6:50 AM
alexfh edited edge metadata.

Looks good with a couple of nits. See the comments.

test/clang-tidy/readability-shrink-to-fit.cpp
48

Any reason for this to be a separate function? Why not just add a test with v2 to g<>?

67

nit: In C++11 you don't need to put a space here.

This revision is now accepted and ready to land.Jan 23 2015, 6:50 AM
alexfh added inline comments.Jan 23 2015, 7:02 AM
test/clang-tidy/readability-shrink-to-fit.cpp
48

Ah, I get it. No need to address this ;)

67

I'll fix it myself and commit the patch.

alexfh closed this revision.Jan 23 2015, 7:14 AM

Committed revision 226912.

Modified before commit:

  • removed a space between >s
  • added a test for dependent types in templates.