[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

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




Comment at: clang-tidy/utils/NamespaceAliaser.cpp:35
+return None;
+
+

nit: Too many empty lines.



Comment at: clang-tidy/utils/NamespaceAliaser.cpp:41
+
+  // TODO(bangert): Doesn't consider the order of declarations.
+  // If we accidentially pick an alias defined later in the function,

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



Comment at: clang-tidy/utils/NamespaceAliaser.h:35
+  llvm::StringRef Namespace,
+  const std::vector );
+

Use llvm::ArrayRef instead.


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Vassil Vassilev via cfe-commits]

v.g.vassilev added a subscriber: rsmith.
v.g.vassilev added inline comments.



Comment at: clang-tidy/utils/UsingInserter.cpp:58
+  if (AlreadyHasUsingDecl) {
+AddedUsing.emplace(NameInFunction(Function, QualifiedName.str()));
+return None;

bkramer wrote:
> v.g.vassilev wrote:
> > 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?
> GCC 4.7 isn't supported anymore by LLVM (rL284497) can we update that bot to 
> a newer version of GCC?
It is managed by @rsmith . He should be able to tell.


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Benjamin Kramer via cfe-commits]

bkramer added inline comments.



Comment at: clang-tidy/utils/UsingInserter.cpp:58
+  if (AlreadyHasUsingDecl) {
+AddedUsing.emplace(NameInFunction(Function, QualifiedName.str()));
+return None;

v.g.vassilev wrote:
> 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?
GCC 4.7 isn't supported anymore by LLVM (rL284497) can we update that bot to a 
newer version of GCC?


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Vassil Vassilev via cfe-commits]

v.g.vassilev added inline comments.



Comment at: clang-tidy/utils/UsingInserter.cpp:58
+  if (AlreadyHasUsingDecl) {
+AddedUsing.emplace(NameInFunction(Function, QualifiedName.str()));
+return None;

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?


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

Yeah, "make -j 32" won't compile binaries in clang-tools-extra repo.

"make check-clang-tools" should work though I don't use make. I usually run
`ninja check-clang-tools` command to build all binaries and run all
lint-tests/unittest in clang-extra-tools. There is a remaining build error
in the unittests, I fixed it and recommitted this patch in r284368.

On Fri, Oct 14, 2016 at 10:26 PM, Julian Bangert  wrote:

> I figured out make clang-tidy. Compiles now (the typedef was the wrong way
> around, and i never noticed because make with the default target continued
> to work).
>
>
> Updated the diff.
>
> On Fri, Oct 14, 2016 at 12:49 PM Julian Bangert 
> wrote:
>
>> Apologies for the breakage. I investigated and it turns out my
>> open-source checkout does not build clang-tidy. I have checked out llvm
>> into ~/llvm, clang into ~/llvm/tools/clang and clang-extra-tools into
>> ~/llvm/tools/clang/tools/extra.
>> In ~/llvm-build I run cmake ../llvm && make -j 32 and it doesn't compile
>> clang tidy. I also tried  cmake -DCLANG_ENABLE_STATIC_ANALYZER=true
>> ../llvm/ but that doesn't make a difference. Any ideas?
>>
>>
>> On Wed, Oct 12, 2016 at 1:34 AM Haojian Wu  wrote:
>>
>> hokein added a comment.
>>
>> @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? )
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D24997
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

I figured out make clang-tidy. Compiles now (the typedef was the wrong way
around, and i never noticed because make with the default target continued
to work).


Updated the diff.

On Fri, Oct 14, 2016 at 12:49 PM Julian Bangert  wrote:

> Apologies for the breakage. I investigated and it turns out my open-source
> checkout does not build clang-tidy. I have checked out llvm into ~/llvm,
> clang into ~/llvm/tools/clang and clang-extra-tools into
> ~/llvm/tools/clang/tools/extra.
> In ~/llvm-build I run cmake ../llvm && make -j 32 and it doesn't compile
> clang tidy. I also tried  cmake -DCLANG_ENABLE_STATIC_ANALYZER=true
> ../llvm/ but that doesn't make a difference. Any ideas?
>
>
> On Wed, Oct 12, 2016 at 1:34 AM Haojian Wu  wrote:
>
> hokein added a comment.
>
> @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? )
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D24997
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

jbangert removed rL LLVM as the repository for this revision.
jbangert updated this revision to Diff 74737.
Herald added subscribers: mgorny, beanz.

https://reviews.llvm.org/D24997

Files:
  clang-tidy/utils/ASTUtils.cpp
  clang-tidy/utils/ASTUtils.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/NamespaceAliaser.cpp
  clang-tidy/utils/NamespaceAliaser.h
  clang-tidy/utils/UsingInserter.cpp
  clang-tidy/utils/UsingInserter.h
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/NamespaceAliaserTest.cpp
  unittests/clang-tidy/UsingInserterTest.cpp

Index: unittests/clang-tidy/UsingInserterTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/UsingInserterTest.cpp
@@ -0,0 +1,115 @@
+//=== UsingInserterTest.cpp - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/UsingInserter.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Replace all function calls with calls to foo::func. Inserts using
+// declarations as necessary. This checker is for testing only. It
+// can only run on one test case (e.g. wih one SourceManager).
+class InsertUsingCheck : public clang::tidy::ClangTidyCheck {
+public:
+  using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+Finder->addMatcher(clang::ast_matchers::callExpr().bind("foo"), this);
+  }
+  void
+  check(const clang::ast_matchers::MatchFinder::MatchResult ) override {
+if (!Inserter)
+  Inserter.reset(new UsingInserter(*Result.SourceManager,
+   Result.Context->getLangOpts()));
+
+const clang::CallExpr *Call =
+Result.Nodes.getNodeAs("foo");
+assert(Call != nullptr && "Did not find node \"foo\"");
+auto Hint =
+Inserter->createUsingDeclaration(*Result.Context, *Call, "::foo::func");
+
+if (Hint.hasValue())
+  diag(Call->getLocStart(), "Fix for testing") << Hint.getValue();
+
+diag(Call->getLocStart(), "insert call")
+<< clang::FixItHint::CreateReplacement(
+   Call->getCallee()->getSourceRange(),
+   Inserter->getShortName(*Result.Context, *Call, "::foo::func"));
+  }
+
+private:
+  std::unique_ptr Inserter;
+};
+
+template 
+std::string runChecker(StringRef Code, int ExpectedWarningCount) {
+  std::map AdditionalFileContents = {{"foo.h",
+"namespace foo {\n"
+"namespace bar {\n"
+"}\n"
+"void func() { }\n"
+"}"}};
+  std::vector errors;
+
+  std::string result =
+  test::runCheckOnCode(Code, , "foo.cc", None,
+  ClangTidyOptions(), AdditionalFileContents);
+
+  EXPECT_EQ(ExpectedWarningCount, errors.size());
+  return result;
+}
+
+TEST(UsingInserterTest, ReusesExisting) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"using ::foo::func;\n"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "using ::foo::func;\n"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, ReusesExistingGlobal) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"using ::foo::func;\n"
+"namespace {"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "using ::foo::func;\n"
+ "namespace {"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, AvoidsConflict) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"void f() { int func; ::foo::func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "void f() { int func; f(); }"
+ "}",
+ 1));

Re: [PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

Apologies for the breakage. I investigated and it turns out my open-source
checkout does not build clang-tidy. I have checked out llvm into ~/llvm,
clang into ~/llvm/tools/clang and clang-extra-tools into
~/llvm/tools/clang/tools/extra.
In ~/llvm-build I run cmake ../llvm && make -j 32 and it doesn't compile
clang tidy. I also tried  cmake -DCLANG_ENABLE_STATIC_ANALYZER=true
../llvm/ but that doesn't make a difference. Any ideas?


On Wed, Oct 12, 2016 at 1:34 AM Haojian Wu  wrote:

> hokein added a comment.
>
> @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? )
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D24997
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

hokein added a comment.

@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? )


Repository:
  rL LLVM

https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL283981: [ClangTidy] Add UsingInserter and NamespaceAliaser 
(authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D24997?vs=74311=74338#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24997

Files:
  clang-tools-extra/trunk/clang-tidy/utils/ASTUtils.cpp
  clang-tools-extra/trunk/clang-tidy/utils/ASTUtils.h
  clang-tools-extra/trunk/clang-tidy/utils/NamespaceAliaser.cpp
  clang-tools-extra/trunk/clang-tidy/utils/NamespaceAliaser.h
  clang-tools-extra/trunk/clang-tidy/utils/UsingInserter.cpp
  clang-tools-extra/trunk/clang-tidy/utils/UsingInserter.h
  clang-tools-extra/trunk/unittests/clang-tidy/NamespaceAliaserTest.cpp
  clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp

Index: clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp
@@ -0,0 +1,115 @@
+//=== UsingInserterTest.cpp - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/UsingInserter.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Replace all function calls with calls to foo::func. Inserts using
+// declarations as necessary. This checker is for testing only. It
+// can only run on one test case (e.g. wih one SourceManager).
+class InsertUsingCheck : public clang::tidy::ClangTidyCheck {
+public:
+  using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+Finder->addMatcher(clang::ast_matchers::callExpr().bind("foo"), this);
+  }
+  void
+  check(const clang::ast_matchers::MatchFinder::MatchResult ) override {
+if (!Inserter)
+  Inserter.reset(new UsingInserter(*Result.SourceManager,
+   Result.Context->getLangOpts()));
+
+const clang::CallExpr *Call =
+Result.Nodes.getNodeAs("foo");
+assert(Call != nullptr && "Did not find node \"foo\"");
+auto Hint =
+Inserter->createUsingDeclaration(*Result.Context, *Call, "::foo::func");
+
+if (Hint.hasValue())
+  diag(Call->getLocStart(), "Fix for testing") << Hint.getValue();
+
+diag(Call->getLocStart(), "insert call")
+<< clang::FixItHint::CreateReplacement(
+   Call->getCallee()->getSourceRange(),
+   Inserter->getShortName(*Result.Context, *Call, "::foo::func"));
+  }
+
+private:
+  std::unique_ptr Inserter;
+};
+
+template 
+std::string runChecker(StringRef Code, int ExpectedWarningCount) {
+  std::map AdditionalFileContents = {{"foo.h",
+"namespace foo {\n"
+"namespace bar {\n"
+"}\n"
+"void func() { }\n"
+"}"}};
+  std::vector errors;
+
+  std::string result =
+  test::runCheckOnCode(Code, , "foo.cc", None,
+  ClangTidyOptions(), AdditionalFileContents);
+
+  EXPECT_EQ(ExpectedWarningCount, errors.size());
+  return result;
+}
+
+TEST(UsingInserterTest, ReusesExisting) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"using ::foo::func;\n"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "using ::foo::func;\n"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, ReusesExistingGlobal) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"using ::foo::func;\n"
+"namespace {"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "using ::foo::func;\n"
+ "namespace {"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, AvoidsConflict) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

jbangert marked an inline comment as done.
jbangert added a comment.

Thanks for the review!


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

jbangert updated this revision to Diff 74311.

https://reviews.llvm.org/D24997

Files:
  clang-tidy/utils/ASTUtils.cpp
  clang-tidy/utils/ASTUtils.h
  clang-tidy/utils/NamespaceAliaser.cpp
  clang-tidy/utils/NamespaceAliaser.h
  clang-tidy/utils/UsingInserter.cpp
  clang-tidy/utils/UsingInserter.h
  unittests/clang-tidy/NamespaceAliaserTest.cpp
  unittests/clang-tidy/UsingInserterTest.cpp

Index: unittests/clang-tidy/UsingInserterTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/UsingInserterTest.cpp
@@ -0,0 +1,115 @@
+//=== UsingInserterTest.cpp - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/UsingInserter.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Replace all function calls with calls to foo::func. Inserts using
+// declarations as necessary. This checker is for testing only. It
+// can only run on one test case (e.g. wih one SourceManager).
+class InsertUsingCheck : public clang::tidy::ClangTidyCheck {
+public:
+  using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+Finder->addMatcher(clang::ast_matchers::callExpr().bind("foo"), this);
+  }
+  void
+  check(const clang::ast_matchers::MatchFinder::MatchResult ) override {
+if (!Inserter)
+  Inserter.reset(new UsingInserter(*Result.SourceManager,
+   Result.Context->getLangOpts()));
+
+const clang::CallExpr *Call =
+Result.Nodes.getNodeAs("foo");
+assert(Call != nullptr && "Did not find node \"foo\"");
+auto Hint =
+Inserter->createUsingDeclaration(*Result.Context, *Call, "::foo::func");
+
+if (Hint.hasValue())
+  diag(Call->getLocStart(), "Fix for testing") << Hint.getValue();
+
+diag(Call->getLocStart(), "insert call")
+<< clang::FixItHint::CreateReplacement(
+   Call->getCallee()->getSourceRange(),
+   Inserter->getShortName(*Result.Context, *Call, "::foo::func"));
+  }
+
+private:
+  std::unique_ptr Inserter;
+};
+
+template 
+std::string runChecker(StringRef Code, int ExpectedWarningCount) {
+  std::map AdditionalFileContents = {{"foo.h",
+"namespace foo {\n"
+"namespace bar {\n"
+"}\n"
+"void func() { }\n"
+"}"}};
+  std::vector errors;
+
+  std::string result =
+  test::runCheckOnCode(Code, , "foo.cc", None,
+  ClangTidyOptions(), AdditionalFileContents);
+
+  EXPECT_EQ(ExpectedWarningCount, errors.size());
+  return result;
+}
+
+TEST(UsingInserterTest, ReusesExisting) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"using ::foo::func;\n"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "using ::foo::func;\n"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, ReusesExistingGlobal) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"using ::foo::func;\n"
+"namespace {"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "using ::foo::func;\n"
+ "namespace {"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, AvoidsConflict) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"void f() { int func; ::foo::func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "void f() { int func; f(); }"
+ "}",
+ 1));
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: unittests/clang-tidy/NamespaceAliaserTest.cpp

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM with one more nit.




Comment at: clang-tidy/utils/ASTUtils.h:25
+
+#endif

missing a trailing comment  `// LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H`


https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

jbangert updated this revision to Diff 74187.
jbangert marked 15 inline comments as done.

https://reviews.llvm.org/D24997

Files:
  clang-tidy/utils/ASTUtils.cpp
  clang-tidy/utils/ASTUtils.h
  clang-tidy/utils/NamespaceAliaser.cpp
  clang-tidy/utils/NamespaceAliaser.h
  clang-tidy/utils/UsingInserter.cpp
  clang-tidy/utils/UsingInserter.h
  unittests/clang-tidy/NamespaceAliaserTest.cpp
  unittests/clang-tidy/UsingInserterTest.cpp

Index: unittests/clang-tidy/UsingInserterTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/UsingInserterTest.cpp
@@ -0,0 +1,115 @@
+//=== UsingInserterTest.cpp - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/UsingInserter.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Replace all function calls with calls to foo::func. Inserts using
+// declarations as necessary. This checker is for testing only. It
+// can only run on one test case (e.g. wih one SourceManager).
+class InsertUsingCheck : public clang::tidy::ClangTidyCheck {
+public:
+  using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+Finder->addMatcher(clang::ast_matchers::callExpr().bind("foo"), this);
+  }
+  void
+  check(const clang::ast_matchers::MatchFinder::MatchResult ) override {
+if (!Inserter)
+  Inserter.reset(new UsingInserter(*Result.SourceManager,
+   Result.Context->getLangOpts()));
+
+const clang::CallExpr *Call =
+Result.Nodes.getNodeAs("foo");
+assert(Call != nullptr && "Did not find node \"foo\"");
+auto Hint =
+Inserter->createUsingDeclaration(*Result.Context, *Call, "::foo::func");
+
+if (Hint.hasValue())
+  diag(Call->getLocStart(), "Fix for testing") << Hint.getValue();
+
+diag(Call->getLocStart(), "insert call")
+<< clang::FixItHint::CreateReplacement(
+   Call->getCallee()->getSourceRange(),
+   Inserter->getShortName(*Result.Context, *Call, "::foo::func"));
+  }
+
+private:
+  std::unique_ptr Inserter;
+};
+
+template 
+std::string runChecker(StringRef Code, int ExpectedWarningCount) {
+  std::map AdditionalFileContents = {{"foo.h",
+"namespace foo {\n"
+"namespace bar {\n"
+"}\n"
+"void func() { }\n"
+"}"}};
+  std::vector errors;
+
+  std::string result =
+  test::runCheckOnCode(Code, , "foo.cc", None,
+  ClangTidyOptions(), AdditionalFileContents);
+
+  EXPECT_EQ(ExpectedWarningCount, errors.size());
+  return result;
+}
+
+TEST(UsingInserterTest, ReusesExisting) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"using ::foo::func;\n"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "using ::foo::func;\n"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, ReusesExistingGlobal) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"using ::foo::func;\n"
+"namespace {"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "using ::foo::func;\n"
+ "namespace {"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, AvoidsConflict) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"void f() { int func; ::foo::func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "void f() { int func; f(); }"
+ "}",
+ 1));
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
Index: unittests/clang-tidy/NamespaceAliaserTest.cpp

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

hokein added a comment.

Looks almost fine, a few code-style comments.



> ASTUtils.h:11
> +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H
> +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H
> +#include "clang/AST/AST.h"

A blank line between define guard and include.

> ASTUtils.h:17
> +namespace utils {
> +const FunctionDecl *getSurroundingFunction(ASTContext ,
> +   const Stmt );

Could you add a few documentation describing this interface?

> ASTUtils.h:21
> +} // namespace tidy
> +} // namespace clang
> +#endif

A blank line below.

> NamespaceAliaser.cpp:8
> +//
> +//===--===//
> +#include "NamespaceAliaser.h"

A blank line.

> NamespaceAliaser.cpp:14
> +#include "clang/ASTMatchers/ASTMatchers.h"
> +#include "clang/Lex/Lexer.h"
> +namespace clang {

A blank line below.

> NamespaceAliaser.cpp:41
> +
> +  // TODO(bangert): Doesn't consider the order of declarations.
> +  // If we accidentially pick an alias defined later in the function,

FIXME.

> NamespaceAliaser.cpp:44
> +  // the output won't compile.
> +  // TODO(bangert): Also doesn't consider file or class-scope aliases.
> +

FIXME.

> NamespaceAliaser.cpp:85
> +   StringRef Namespace) const {
> +  auto *Function = getSurroundingFunction(Context, Statement);
> +  auto FunctionAliases = AddedAliases.find(Function);

I'd prefer to declare it `const auto *`.

> NamespaceAliaser.h:8
> +//
> +//===--===//
> +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NAMESPACEALIASER_H

A blank line.

> NamespaceAliaser.h:21
> +namespace tidy {
> +namespace utils {
> +// This class creates function-level namespace aliases.

A blank line below.

> NamespaceAliaser.h:28
> +  // Statement. Picks the first available name from \p Abbreviations.
> +  // Returns ``llvm::None`` if an alias already exists or these is an error.
> +  llvm::Optional

s/these/there?

> NamespaceAliaser.h:41
> +  const SourceManager 
> +  std::map> AddedAliases;
> +};

Use `llvm::DenseMap` instead.

> UsingInserter.cpp:49
> +
> +  // TODO(bangert): This declaration could be masked. Investigate if
> +  // there is a way to avoid using Sema.

Use `FIXME` instead of `TODO(bangert).`

> UsingInserter.h:23
> +
> +// UsingInserter adds function-level using declarations.
> +class UsingInserter {

Could you elaborate a bit more?

> UsingInserter.h:41
> +  const SourceManager 
> +  std::set AddedUsing;
> +};

It'd be clearer to use a typedef for `std::pair` here.

https://reviews.llvm.org/D24997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Julian Bangert via cfe-commits]

jbangert created this revision.
jbangert added a reviewer: alexfh.
jbangert added a subscriber: cfe-commits.
jbangert added a project: clang-tools-extra.

This adds helper classes to add using declaractions and namespace aliases to 
function bodies. These help making function calls to deeply nested functions 
concise (e.g. when calling helpers in a refactoring)

https://reviews.llvm.org/D24997

Files:
  clang-tidy/utils/ASTUtils.cpp
  clang-tidy/utils/ASTUtils.h
  clang-tidy/utils/NamespaceAliaser.cpp
  clang-tidy/utils/NamespaceAliaser.h
  clang-tidy/utils/UsingInserter.cpp
  clang-tidy/utils/UsingInserter.h
  unittests/clang-tidy/NamespaceAliaserTest.cpp
  unittests/clang-tidy/UsingInserterTest.cpp

Index: unittests/clang-tidy/UsingInserterTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/UsingInserterTest.cpp
@@ -0,0 +1,115 @@
+//=== UsingInserterTest.cpp - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/UsingInserter.h"
+
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// Replace all function calls with calls to foo::func. Inserts using
+// declarations as necessary. This checker is for testing only. It
+// can only run on one test case (e.g. wih one SourceManager).
+class InsertUsingCheck : public clang::tidy::ClangTidyCheck {
+public:
+  using clang::tidy::ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(clang::ast_matchers::MatchFinder *Finder) override {
+Finder->addMatcher(clang::ast_matchers::callExpr().bind("foo"), this);
+  }
+  void
+  check(const clang::ast_matchers::MatchFinder::MatchResult ) override {
+if (!Inserter)
+  Inserter.reset(new UsingInserter(*Result.SourceManager,
+   Result.Context->getLangOpts()));
+
+const clang::CallExpr *Call =
+Result.Nodes.getNodeAs("foo");
+assert(Call != nullptr && "Did not find node \"foo\"");
+auto Hint =
+Inserter->createUsingDeclaration(*Result.Context, *Call, "::foo::func");
+
+if (Hint.hasValue())
+  diag(Call->getLocStart(), "Fix for testing") << Hint.getValue();
+
+diag(Call->getLocStart(), "insert call")
+<< clang::FixItHint::CreateReplacement(
+   Call->getCallee()->getSourceRange(),
+   Inserter->getShortName(*Result.Context, *Call, "::foo::func"));
+  }
+
+private:
+  std::unique_ptr Inserter;
+};
+
+template 
+std::string runChecker(StringRef Code, int ExpectedWarningCount) {
+  std::map AdditionalFileContents = {{"foo.h",
+"namespace foo {\n"
+"namespace bar {\n"
+"}\n"
+"void func() { }\n"
+"}"}};
+  std::vector errors;
+
+  std::string result =
+  test::runCheckOnCode(Code, , "foo.cc", None,
+  ClangTidyOptions(), AdditionalFileContents);
+
+  EXPECT_EQ(ExpectedWarningCount, errors.size());
+  return result;
+}
+
+TEST(UsingInserterTest, ReusesExisting) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"using ::foo::func;\n"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+ "using ::foo::func;\n"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, ReusesExistingGlobal) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"using ::foo::func;\n"
+"namespace {"
+"void f() { func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "using ::foo::func;\n"
+ "namespace {"
+ "void f() { f(); }"
+ "}",
+ 1));
+}
+
+TEST(UsingInserterTest, AvoidsConflict) {
+  EXPECT_EQ("#include \"foo.h\"\n"
+"namespace {"
+"void f() { int func; ::foo::func(); }"
+"}",
+runChecker("#include \"foo.h\"\n"
+ "namespace {"
+