This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Handle using-decls with more than one shadow decl.
ClosedPublic

Authored by hokein on May 19 2016, 6:17 AM.

Diff Detail

Event Timeline

hokein updated this revision to Diff 57778.May 19 2016, 6:17 AM
hokein retitled this revision from to [clang-tidy] Handle using-decls with more than one shadow decl..
hokein updated this object.
hokein added a reviewer: alexfh.
hokein added subscribers: djasper, cfe-commits.
alexfh requested changes to this revision.May 19 2016, 6:35 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
22

This method assumes a rather non-trivial definition of "valid". Please add a comment what exactly this method does.

Also, static is preferred to anonymous namespaces for functions in LLVM style (http://llvm.org/docs/CodingStandards.html#anonymous-namespaces):

... make anonymous namespaces as small as possible, and only use them for class declarations. ...
29

Debug output?

103

.count() for sets is as effective, but results in clearer code.

clang-tidy/misc/UnusedUsingDeclsCheck.h
39

SmallPtrSet should be more efficient here.

test/clang-tidy/misc-unused-using-decls.cpp
86

Not for this patch, but it makes sense to remove trailing comments on deleted lines.

This revision now requires changes to proceed.May 19 2016, 6:35 AM
hokein updated this revision to Diff 57785.May 19 2016, 7:26 AM
hokein edited edge metadata.
hokein marked 4 inline comments as done.

Address comments.

hokein updated this revision to Diff 57790.May 19 2016, 7:32 AM
hokein edited edge metadata.

Forgot a comments.

hokein marked an inline comment as done.May 19 2016, 7:32 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
22

I have renamed to IsCheckable, but it doesn't make more sense here. Do you have better suggestion on the function name?

test/clang-tidy/misc-unused-using-decls.cpp
86

Will do it in a follow-up.

alexfh accepted this revision.May 19 2016, 5:07 PM
alexfh edited edge metadata.

LG with a couple of nits.

clang-tidy/misc/UnusedUsingDeclsCheck.cpp
22

ShouldCheckDecl might convey the idea better.

59

It's not iterator, so It is a confusing name. Something along the lines of Shadow or UsingShadow should be better.

This revision is now accepted and ready to land.May 19 2016, 5:07 PM
hokein updated this revision to Diff 57904.May 20 2016, 1:18 AM
hokein edited edge metadata.

Address more comments.

hokein marked 2 inline comments as done.May 20 2016, 1:19 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
59

Actually, the Using->shadows() returns an iterator range, but I'm fine renaming it here.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
alexfh added inline comments.May 20 2016, 1:56 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
59

Sure, it returns a llvm::iterator_range<shadow_iterator>, which is just a range adaptor for a pair of iterators. It's not a "collection of iterators", it's a "collection, defined by a pair of iterators". If you iterate over it using a range-based for loop, you get whatever is pointed by the iterators, not iterators.

hokein added inline comments.May 20 2016, 2:14 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
59

I see it now. Thanks for the explanations :-).