This is an archive of the discontinued LLVM Phabricator instance.

A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
Needs ReviewPublic

Authored by mclow.lists on Dec 1 2015, 10:26 AM.

Details

Summary

Motivation: LLVM has many overloads of std::swap for its own types. This is technically forbidden by the standard, but is pervasive in the LLVM code base. The correct fix for this is to put them in the same namespace as the thing that they are swapping - but do do this, we have to find (and fix) all the places where we call std::swap explicitly, and let ADL do the finding instead.

The basic transform is:

std::swap(x, y) --> { using std::swap; swap(x, y); }

It would be better if the checking for builtin types and pointers were in the matcher instead of the code.

Thanks to Manuel and Aaron for helping with this

Diff Detail

Event Timeline

mclow.lists retitled this revision from to A new clang-tidy module to find calls to `std::swap`, and change them to use ADL.
mclow.lists updated this object.
mclow.lists added a subscriber: cfe-commits.

Also, even though the --fix done by the tool is (almost) correct, the display of the fixit hint by clang is really confusing, even wrong

mclow.lists updated this revision to Diff 41559.Dec 1 2015, 1:46 PM
mclow.lists updated this object.

Suggestions from @AaronBallman; now does the substitution correctly, and passes the tests.

Note: The diagnostics that clang emits are still whacko.

I'm wondering if there isn't an existing module that would be a good fit for this check. Why not in the modernize module?

rsmith edited edge metadata.Dec 1 2015, 5:24 PM

I'm wondering if there isn't an existing module that would be a good fit for this check. Why not in the modernize module?

This isn't really modernization, it's a bug fix. ADL has always been the right way to find swap.

clang-tidy/misc/StdSwapCheck.cpp
25

Is there somewhere more central where this can live?

68

I think the two callee checks can be folded into one. Can you put the check that the namespace name is "std" in here too?

68

Ignoring parens here will lead to your fixit not working. Instead of trying to replace the "std::" with "{ using std::swap; ", maybe it'd be better and safer to replace the entire callee with "{ using std::swap; swap"?

78

Presumably this should also be checking that the parent of nsNode is the translation unit; we're not looking for a std nested within some other namespace here.

79–83

These lines seem underindented.

85–92

This will do bad things if the std::swap is not at the start of the enclosing statement (or if there is no enclosing statement).

docs/clang-tidy/checks/list.rst
35

Why is it called StdSwap here and std-swap below?

docs/clang-tidy/checks/misc-StdSwap.rst
6 ↗(On Diff #41559)

practices -> practice
ADL lookup -> ADL / argument dependent lookup / argument dependent name lookup
swap -> swap
std -> std
"using" statement -> using declaration

14 ↗(On Diff #41559)

brackets -> braces [the term "brackets" is ambiguous and implies different things in different dialects]

aaron.ballman added inline comments.Dec 2 2015, 7:00 AM
clang-tidy/misc/StdSwapCheck.cpp
25

If it is useful to multiple checkers, it could live in clangTidyUtils, or were you thinking of something more general for clang itself?

87

Can we get an && message in here as to what is being asserted?

90

I wonder if there is a way to guard against code like:

SomeStruct S1, S2;

if (something) {  std::swap(S1, S2); }

from becoming:

SomeStruct S1, S2;

if (something) {{ using std::swap; swap(S3, S4); }}

Also, should the fixit be suppressed under some circumstances?

// Is the replacement always safe for all macro expansions?
#define SWAP(a, b) std::swap(a, b)

// I should probably be punished for considering this code
for (;;std::swap(a, b)) ;
if (std::swap(a, b), (bool)a) ;
clang-tidy/misc/StdSwapCheck.h
19

Can this FIXME be fixed? ;-)

docs/clang-tidy/checks/misc-StdSwap.rst
1 ↗(On Diff #41559)

This is incorrect, as is the file name (at least, compared to the comments in StdSwapCheck.h. Can this (and the file) be renamed to misc-std-swap instead?

test/clang-tidy/misc-StdSwap.cpp
4

It would be good to not have the #include <utility> here -- for instance, I think that this test will fail on Windows with MSVC if the only MSVC STL headers that can be found are from VS 2015 because there's no -fms-compatibility-version=19 option being used.

62

It would also be good to have a test that checks bar::std::swap(a, b) doesn't get flagged.

mclow.lists updated this revision to Diff 41626.Dec 2 2015, 7:59 AM
mclow.lists updated this object.
mclow.lists edited edge metadata.
mclow.lists marked 7 inline comments as done.

Fixed all the simple things that people commented on.

clang-tidy/misc/StdSwapCheck.cpp
25

I'd love to have this in a library somewhere; I found a discussion about this from a year ago; Manuel seemed to think that it was a good idea, but nothing apparently came of that. I lifted this code from llvm/tools/clang/lib/ARCMigrate/Transforms.cpp

test/clang-tidy/misc-StdSwap.cpp
4

The reason that <utility> is included here is that that is where swap is declared.

klimek added inline comments.
clang-tidy/misc/StdSwapCheck.cpp
26

We should put that somewhere into Tooling/Core. Adding Benjamin who is our master of Yaks :D

aaron.ballman added inline comments.Dec 2 2015, 8:17 AM
clang-tidy/misc/StdSwapCheck.cpp
87

Extra space at the end of the assert before the closing paren.

clang-tidy/misc/StdSwapCheck.h
20

Strange indentation on the second line of the comment.

test/clang-tidy/misc-StdSwap.cpp
5

The reason that <utility> is included here is that that is where swap is declared.

The usual approach we take in our tests is to declare the STL functionality inside of the source file being tested. For instance, see misc-move-constructor-init.cpp. Another approach, if you require the declaration to be in a header file (which you don't appear to from the checker) is to create a local test file and #include it with "header" instead of <header>. Relying on whatever STL happens to be found by header search logic makes the tests highly fragile (and bots will happily chirp at you when you do this).

mclow.lists added inline comments.Dec 2 2015, 10:01 AM
clang-tidy/misc/StdSwapCheck.cpp
69

I believe that if you do that, you lose the arguments as well.

std::swap(x,y);  -> { using ::std::swap; swap; }
clang-tidy/misc/StdSwapCheck.h
20

I like that indentation :-)
It indicates that this is a continuation of the previous line.

mclow.lists updated this revision to Diff 41662.Dec 2 2015, 1:13 PM

More tests; incorporated some of the suggestions for making sure we don't step on other people's namespaces named std.

mclow.lists updated this revision to Diff 41667.Dec 2 2015, 1:33 PM

Richard clued me in to the cool method isStdNamespace(), which made the code simpler.

mclow.lists marked 5 inline comments as done.Dec 2 2015, 1:35 PM

(tangential: Should we just have a utility function llvm::adl_swap that we
could use everywhere rather than having to write two lines for every
caller? (& have a variant of this check that knows to suggest that in
LLVM?))

I ran this on LLVM + clang, and it changed calls in 61 files. The changed LLVM codebase compiled successfully, and passed all the tests.

Re: @dblaikie's comment, I'd rather call such a beast llvm::swap, and it would have to go into a header file that everyone already includes.

alexfh added a subscriber: alexfh.Dec 28 2015, 6:26 AM

Found this accidentally, since I don't read all of cfe-commits normally. Would be nice if you could add me to reviewers on clang-tidy-related patches. Thanks!

(tangential: Should we just have a utility function llvm::adl_swap that we
could use everywhere rather than having to write two lines for every
caller? (& have a variant of this check that knows to suggest that in
LLVM?))

FWIW, if this seems to be useful for other codebases, the name of this function could be made configurable (together with the header this function comes from - clang-tidy checks can use clang::tidy::IncludeInserter to insert includes in a proper way).

aaron.ballman edited edge metadata.Jan 6 2016, 6:19 AM

I notice there are a few other comments from reviewers that have not been addressed -- is there a newer version of the patch that hasn't been uploaded yet, or are you looking for further information on some of the comments?

clang-tidy/misc/StdSwapCheck.cpp
27

We should put that somewhere into Tooling/Core. Adding Benjamin who is our master of Yaks :D

At the very least, this could live in clangTidyUtils, possibly in LexerUtils.h.

66

Please remove commented out code.

78

Please remove commented-out code (here and elsewhere).

clang-tidy/misc/StdSwapCheck.h
21

I like that indentation :-)
It indicates that this is a continuation of the previous line.

It's not the style of commenting that we use in the rest of the project, however.

jbcoe added a subscriber: jbcoe.Feb 12 2016, 2:00 PM
Prazek added a subscriber: Prazek.Jul 5 2016, 8:53 AM
Prazek added inline comments.
test/clang-tidy/misc-StdSwap.cpp
8–12

template thing

Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 10:27 AM