This is an archive of the discontinued LLVM Phabricator instance.

[ClangTidy] Add UsingInserter and NamespaceAliaser
ClosedPublic

Authored by jbangert on Sep 27 2016, 6:29 PM.

Diff Detail

Event Timeline

jbangert updated this revision to Diff 72751.Sep 27 2016, 6:29 PM
jbangert retitled this revision from to [ClangTidy] Add UsingInserter and NamespaceAliaser.
jbangert updated this object.
jbangert added a reviewer: alexfh.
jbangert added a project: Restricted Project.
jbangert added a subscriber: cfe-commits.
hokein edited edge metadata.Oct 4 2016, 3:17 AM

Looks almost fine, a few code-style comments.

clang-tidy/utils/ASTUtils.h
12

A blank line between define guard and include.

18

Could you add a few documentation describing this interface?

22

A blank line below.

clang-tidy/utils/NamespaceAliaser.cpp
9

A blank line.

15

A blank line below.

42

FIXME.

45

FIXME.

86

I'd prefer to declare it const auto *.

clang-tidy/utils/NamespaceAliaser.h
9

A blank line.

22

A blank line below.

29

s/these/there?

42

Use llvm::DenseMap instead.

clang-tidy/utils/UsingInserter.cpp
50

Use FIXME instead of TODO(bangert).

clang-tidy/utils/UsingInserter.h
24

Could you elaborate a bit more?

42

It'd be clearer to use a typedef for std::pair<const FunctionDecl *, std::string> here.

jbangert updated this revision to Diff 74187.Oct 10 2016, 4:53 PM
jbangert edited edge metadata.
jbangert marked 15 inline comments as done.
hokein accepted this revision.Oct 11 2016, 1:07 AM
hokein edited edge metadata.

LGTM with one more nit.

clang-tidy/utils/ASTUtils.h
26

missing a trailing comment // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H

This revision is now accepted and ready to land.Oct 11 2016, 1:07 AM
jbangert updated this revision to Diff 74311.Oct 11 2016, 5:02 PM
jbangert edited edge metadata.
jbangert marked an inline comment as done.

Thanks for the review!

This revision was automatically updated to reflect the committed changes.

@jbangert, your patch broke the buildbot, and I reverted it in r283985. You need to add the new source files to the CMakefile. (I saw other compilation errors after I updated the cmake files, Could you take a look on it? )

jbangert updated this revision to Diff 74737.Oct 14 2016, 1:26 PM
jbangert removed rL LLVM as the repository for this revision.
v.g.vassilev added inline comments.
clang-tidy/utils/UsingInserter.cpp
58

Using emplace seems to break our modules libstdc++ 4.7 builds: http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules-2/builds/6066 . Doesn't that break other 4.7 or earlier builds?

bkramer added inline comments.
clang-tidy/utils/UsingInserter.cpp
58

GCC 4.7 isn't supported anymore by LLVM (rL284497) can we update that bot to a newer version of GCC?

v.g.vassilev added inline comments.
clang-tidy/utils/UsingInserter.cpp
58

It is managed by @rsmith . He should be able to tell.

alexfh edited edge metadata.Oct 27 2016, 7:31 AM

A few late comments (seems to be relevant to the revision finally committed after all fixes).

clang-tidy/utils/NamespaceAliaser.cpp
36

nit: Too many empty lines.

42

Please use FIXME instead of TODO(...).

clang-tidy/utils/NamespaceAliaser.h
35

Use llvm::ArrayRef instead.