[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-08-30 Thread Sergey Tsatsulin via Phabricator via cfe-commits 
tsatsulin marked an inline comment as done.
tsatsulin added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:83
+
+  void Ifndef(SourceLocation Loc, const Token ,
+  const MacroDefinition ) override {

The body of this method is the same as `Ifdef`. Please check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-07-29 Thread Eric Fiselier via Phabricator via cfe-commits 
EricWF closed this revision.
EricWF added a comment.

Committed in r367263. Thanks for the change, sorry about the delay.

I'll go watch the bots now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-28 Thread Eric Fiselier via Phabricator via cfe-commits 
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

I don't have any other comments.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-27 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni marked 4 inline comments as done.
astrelni added a comment.

In D62977#1560879 , @lebedev.ri wrote:

> Looks reasonable. I did not review the check itself though.
>  Are `test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp` and 
> `test/clang-tidy/google-upgrade-googletest-case.cpp ` identical other than 
> the included header and expected output?
>  I'd recommend to condense it into a single file, and just have two `RUN` 
> lines each one checking different message prefixes


The nosuite test was a strict subset. I've combined them with a few lines that 
needed to be turned via preprocessor branches. Thank you, I actually wasn't 
aware that these tests can have multiple `RUN` lines.




Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:34
+private:
+  std::unordered_set MatchedTemplateLocations;
+};

lebedev.ri wrote:
> Have you tried `llvm::DenseSet` instead?
> This //may// not matter *here*, but `std::unordered_set` usually results in 
> horrible perf.
Thanks, I wasn't aware of what is available.



Comment at: 
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp:9
+void DummyFixTarget() {}
+// CHECK-FIXES: void DummyFixTarget() {}
+

lebedev.ri wrote:
> Hm?
I added a comment to better explain this in the combined test. 
check_clang_tidy.py asserts that there is at least one message, fix or note 
comment present in the file. If there isn't one, the script returns without 
even running the test. I couldn't see an option to pass that would turn of this 
check, please let me know if you are aware of one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-27 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 206889.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,1016 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+// RUN: %check_clang_tidy -check-suffix=NOSUITE %s google-upgrade-googletest-case %t -- -- -DNOSUITE -I%S/Inputs/gtest/nosuite
+
+#include "gtest/gtest.h"
+
+// When including a version of googletest without the replacement names, this
+// check should not produce any diagnostics. The following dummy fix is present
+// because `check_clang_tidy.py` requires at least one warning, fix or note. 
+void Dummy() {}
+// CHECK-FIXES-NOSUITE: void Dummy() {}
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE(CaseName, Types, ...)
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE_P(SuiteName)
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define REGISTER_TYPED_TEST_CASE_P(SuiteName, ...)
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define INSTANTIATE_TYPED_TEST_CASE_P(Prefix, SuiteName, Types, ...)
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+// testing::Test
+
+class FooTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void SetUpTestSuite();
+  static void TearDownTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void TearDownTestSuite();
+};
+
+void

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-27 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri resigned from this revision.
lebedev.ri added a comment.

Looks reasonable. I did not review the check itself though.
Are `test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp` and 
`test/clang-tidy/google-upgrade-googletest-case.cpp ` identical other than the 
included header and expected output?
I'd recommend to condense it into a single file, and just have two `RUN` lines 
each one checking different message prefixes




Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:34
+private:
+  std::unordered_set MatchedTemplateLocations;
+};

Have you tried `llvm::DenseSet` instead?
This //may// not matter *here*, but `std::unordered_set` usually results in 
horrible perf.



Comment at: 
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp:9
+void DummyFixTarget() {}
+// CHECK-FIXES: void DummyFixTarget() {}
+

Hm?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-27 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni added a comment.

In D62977#1559649 , @lebedev.ri wrote:

> In D62977#1559637 , @astrelni wrote:
>
> > In D62977#1559628 , @lebedev.ri 
> > wrote:
> >
> > > It //sounds// correct, but i don't see accompanying test changes, so i 
> > > can't be sure.
> >
> >
> > The tests as they are cover the positive case in that they would not show 
> > warning or fixes if we didn't find the replacements.
>
>
> Yep
>
> In D62977#1559637 , @astrelni wrote:
>
> > The only way to test the negative is to make a second test with a second 
> > set of mock googletest headers that declare things in the v1.8 way. Is this 
> > what you would prefer?
>
>
> If that is what it takes to get the test coverage, i suppose so.


What do you think of this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-27 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 206855.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,991 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+
+#include "gtest/gtest.h"
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE TYPED_TEST_SUITE
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE_P TYPED_TEST_SUITE_P
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define REGISTER_TYPED_TEST_CASE_P REGISTER_TYPED_TEST_SUITE_P
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define INSTANTIATE_TYPED_TEST_CASE_P INSTANTIATE_TYPED_TEST_SUITE_P
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+// testing::Test
+
+class FooTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void SetUpTestSuite();
+  static void TearDownTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void TearDownTestSuite();
+};
+
+void FooTest::SetUpTestCase() {}
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: void FooTest::SetUpTestSuite() {}
+
+void FooTest::TearDownTestCase() {}
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: void FooTest::TearDownTestSuite() {}
+
+template  class

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-26 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

In D62977#1559637 , @astrelni wrote:

> In D62977#1559628 , @lebedev.ri 
> wrote:
>
> > It //sounds// correct, but i don't see accompanying test changes, so i 
> > can't be sure.
>
>
> The tests as they are cover the positive case in that they would not show 
> warning or fixes if we didn't find the replacements.


Yep

In D62977#1559637 , @astrelni wrote:

> The only way to test the negative is to make a second test with a second set 
> of mock googletest headers that declare things in the v1.8 way. Is this what 
> you would prefer?


If that is what it takes to get the test coverage, i suppose so.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-26 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni added a comment.

In D62977#1559628 , @lebedev.ri wrote:

> It //sounds// correct, but i don't see accompanying test changes, so i can't 
> be sure.


The tests as they are cover the positive case in that they would not show 
warning or fixes if we didn't find the replacements. The only way to test the 
negative is to make a second test with a second set of mock googletest headers 
that declare things in the v1.8 way. Is this what you would prefer?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-26 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

It //sounds// correct, but i don't see accompanying test changes, so i can't be 
sure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-26 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni added a comment.

In D62977#1559344 , @lebedev.ri wrote:

> In D62977#1559334 , @astrelni wrote:
>
> > In D62977#1556346 , @lebedev.ri 
> > wrote:
> >
> > > In D62977#1540184 , @lebedev.ri 
> > > wrote:
> > >
> > > > Without seeing the tests - what version checks does this have?
> > > >  It shouldn't fire if the googletest version is the one before that 
> > > > rename.
> > >
> > >
> > > I don't believe this question was answered.
> >
> >
> > Sorry, the original comment got lost between me updating two patches as I 
> > noticed I did it wrong without seeing your comment.
> >
> > Unfortunately there are no versioning macros in googletest, so I'm not sure 
> > how to keep this check from providing warnings if it is run while depending 
> > on a version that is too early. The new names are in master and will be 
> > part of an upcoming version 1.9 release. I have tried to update the 
> > documentation to clarify which version of googletest this intended for. 
> > Please let me know how you think we should proceed.
>
>
> I'm not fully current on what can and can't be done in clang-tidy, but i 
> suppose if the
>  check has matched something, then it must mean that the googletest headers 
> were parsed.
>  Can you perhaps look in the AST that the new names are present?
>  E.g. the new macros, specified in `getNewMacroName()`.


Yes, makes sense. Let me know what you think of the approach I've added in the 
latest diff. I think it is sufficient to check for the existence of one of the 
new macros in the right file and one new method in each matched class.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-26 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 206720.
astrelni marked 4 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,991 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+
+#include "gtest/gtest.h"
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE TYPED_TEST_SUITE
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define TYPED_TEST_CASE_P TYPED_TEST_SUITE_P
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define REGISTER_TYPED_TEST_CASE_P REGISTER_TYPED_TEST_SUITE_P
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Google Test APIs named with 'case'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Google Test APIs named with 'case'
+#define INSTANTIATE_TYPED_TEST_CASE_P INSTANTIATE_TYPED_TEST_SUITE_P
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+// testing::Test
+
+class FooTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void SetUpTestSuite();
+  static void TearDownTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static void TearDownTestSuite();
+};
+
+void FooTest::SetUpTestCase() {}
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: void FooTest::SetUpTestSuite() {}
+
+void FooTest::TearDownTestCase() {}
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+// CHECK-FIXES: void FooTest::TearDownTestSuite() {}
+
+template  class FooTypedTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Google Test APIs named with 'case'
+  // CHECK-FIXES: static

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-25 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:41-42
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"google-upgrade-googletest-case");
 CheckFactories.registerCheck(

Please keep this sorted alphabetically.



Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:20
+static const llvm::StringRef CheckMessage =
+"Googletest APIs named with 'case' are deprecated; use equivalent APIs "
+"named with 'suite'";

Should this be spelled `Google Test` instead?



Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:107-108
+ Preprocessor *) {
+  PP->addPPCallbacks(
+  llvm::make_unique(this, PP));
+}

No need to register this unless in C++ mode.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-24 Thread Eric Fiselier via Phabricator via cfe-commits 
EricWF added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:30
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 

Just tried building this. I think you're missing an include for 
`UpgradeGoogletestCaseCheck`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-24 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

In D62977#1540184 , @lebedev.ri wrote:

> Without seeing the tests - what version checks does this have?
>  It shouldn't fire if the googletest version is the one before that rename.


I don't believe this question was answered.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-24 Thread Eric Fiselier via Phabricator via cfe-commits 
EricWF added a comment.

Generally this LGTM.
I'll take another pass after the comments are addressed.




Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:19
+
+static const llvm::StringRef CheckMessage =
+"Googletest APIs named with 'case' are deprecated; use equivalent APIs "

Could you rename this to better represent what the diagnostic text is saying, 
rather than just being diagnostic text?
I think it'll help readability below.

Why not put this inside the unnamed namespace instead?

Same with the function below.



Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:201
+ const MatchFinder::MatchResult ) {
+  internal::Matcher IsInsideTemplate =
+  hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl(;

Instead of walking the tree, can't you ask if the Node is dependent in some way?



Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:294
+  } else {
+// This is a match for `TestCase` to `TestSuite` refactoring.
+ReplacementText = "TestSuite";

Maybe add an assertion for this comment?



Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:23
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/google-upgrade-googletest-case.html
+class UpgradeGoogletestCaseCheck : public ClangTidyCheck {

`https` all the things!



Comment at: 
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp:9
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' 
are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);

You probably don't have to test for the full message every time. but I don't 
know it matters either way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 204318.
astrelni added a comment.

Fix diff issues again


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,991 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+
+#include "gtest/gtest.h"
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE TYPED_TEST_SUITE
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE_P TYPED_TEST_SUITE_P
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define REGISTER_TYPED_TEST_CASE_P REGISTER_TYPED_TEST_SUITE_P
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define INSTANTIATE_TYPED_TEST_CASE_P INSTANTIATE_TYPED_TEST_SUITE_P
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+// testing::Test
+
+class FooTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+  // CHECK-FIXES: static void SetUpTestSuite();
+  static void TearDownTestCase();
+  // CHECK-MESSAGES:

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 204317.
astrelni added a comment.

Fix mistake not uploading full diff


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,991 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+
+#include "gtest/gtest.h"
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE TYPED_TEST_SUITE
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE_P TYPED_TEST_SUITE_P
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define REGISTER_TYPED_TEST_CASE_P REGISTER_TYPED_TEST_SUITE_P
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define INSTANTIATE_TYPED_TEST_CASE_P INSTANTIATE_TYPED_TEST_SUITE_P
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+// testing::Test
+
+class FooTest : public testing::Test {
+public:
+  static void SetUpTestCase();
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+  // CHECK-FIXES: static void SetUpTestSuite();
+  static void TearDownTestCase();
+  //

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni marked an inline comment as done.
astrelni added a comment.

In D62977#1533384 , @Eugene.Zelenko 
wrote:

> I think will be good idea to replace //upgrade// with //modernize// to be 
> consistent with similar checks in other module.


I will work on changing this in the next diff update, if your opinion holds. 
The context for currently using "upgrade" is:

a) The modernize module seems to contain checks that are geared at using more 
modern language features as opposed to API breaking changes. With some 
exceptions, they seem to be about changing from old to new practice while both 
ways remain valid code. The goal of this check is to take code that will break 
with future googletest API changes and turn it into working code, allowing 
users to upgrade the version of googletest they use. Maybe upgrade is not the 
best term, but I don't think modernize is a better fit.
b) The check is meant to be consistent with 
abseil-upgrade-duration-conversions, which is aimed to upgrade code before API 
breaking changes. So if we change this check, I would like to change the Abseil 
one as well. Maybe this would be better done together in a follow-up patch?

I don't feel very strongly, just thought it would be good to provide the 
context. Please let me know how you'd like me to proceed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

You need to upload the entire patch, not as compared to the previous patch.
Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 204313.
astrelni marked 2 inline comments as done.
astrelni added a comment.

Fix space.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst


Index: 
clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
+++ clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
@@ -10,7 +10,7 @@
 meanings of "test case" and "test suite" as used by the International
 Software Testing Qualifications Board and ISO 29119.
 
-The affected APIs are :
+The affected APIs are:
 
 - Member functions of ``testing::Test``, ``testing::TestInfo``,
   ``testing::TestEventListener``, ``testing::UnitTest``, and any type 
inheriting


Index: clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
+++ clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
@@ -10,7 +10,7 @@
 meanings of "test case" and "test suite" as used by the International
 Software Testing Qualifications Board and ISO 29119.
 
-The affected APIs are :
+The affected APIs are:
 
 - Member functions of ``testing::Test``, ``testing::TestInfo``,
   ``testing::TestEventListener``, ``testing::UnitTest``, and any type inheriting
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-12 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni updated this revision to Diff 204312.
astrelni added a comment.

Style updates.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

Index: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
===
--- clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
+++ clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
@@ -10,7 +10,6 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_UPGRADEGOOGLETESTCASECHECK_H
 
 #include "../ClangTidyCheck.h"
-
 #include 
 
 namespace clang {
Index: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
+++ clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
@@ -15,13 +15,13 @@
 namespace clang {
 namespace tidy {
 namespace google {
-namespace {
 
-const llvm::StringRef CheckMessage =
+static const llvm::StringRef CheckMessage =
 "Googletest APIs named with 'case' are deprecated; use equivalent APIs "
 "named with 'suite'";
 
-llvm::Optional getNewMacroName(llvm::StringRef MacroName) {
+static llvm::Optional
+getNewMacroName(llvm::StringRef MacroName) {
   std::pair ReplacementMap[] = {
   {"TYPED_TEST_CASE", "TYPED_TEST_SUITE"},
   {"TYPED_TEST_CASE_P", "TYPED_TEST_SUITE_P"},
@@ -38,6 +38,8 @@
   return llvm::None;
 }
 
+namespace {
+
 class UpgradeGoogletestCasePPCallback : public PPCallbacks {
 public:
   UpgradeGoogletestCasePPCallback(UpgradeGoogletestCaseCheck *Check,
@@ -81,7 +83,7 @@
 if (!Replacement)
   return;
 
-StringRef FileName = PP->getSourceManager().getFilename(
+llvm::StringRef FileName = PP->getSourceManager().getFilename(
 MD.getMacroInfo()->getDefinitionLoc());
 if (!FileName.endswith("gtest/gtest-typed-test.h"))
   return;
@@ -165,9 +167,7 @@
   this);
 }
 
-namespace {
-
-llvm::StringRef getNewMethodName(llvm::StringRef CurrentName) {
+static llvm::StringRef getNewMethodName(llvm::StringRef CurrentName) {
   std::pair ReplacementMap[] = {
   {"SetUpTestCase", "SetUpTestSuite"},
   {"TearDownTestCase", "TearDownTestSuite"},
@@ -190,21 +190,22 @@
 }
 
 template 
-bool isInInstantiation(const NodeType ,
-   const MatchFinder::MatchResult ) {
+static bool isInInstantiation(const NodeType ,
+  const MatchFinder::MatchResult ) {
   return !match(isInTemplateInstantiation(), Node, *Result.Context).empty();
 }
 
 template 
-bool isInTemplate(const NodeType ,
-  const MatchFinder::MatchResult ) {
+static bool isInTemplate(const NodeType ,
+ const MatchFinder::MatchResult ) {
   internal::Matcher IsInsideTemplate =
   hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl(;
   return !match(IsInsideTemplate, Node, *Result.Context).empty();
 }
 
-bool derivedTypeHasReplacementMethod(const MatchFinder::MatchResult ,
- llvm::StringRef ReplacementMethod) {
+static bool
+derivedTypeHasReplacementMethod(const MatchFinder::MatchResult ,
+llvm::StringRef ReplacementMethod) {
   const auto *Class = Result.Nodes.getNodeAs("class");
   return !match(cxxRecordDecl(
 unless(isExpansionInFileMatching(
@@ -214,7 +215,8 @@
   .empty();
 }
 
-CharSourceRange getAliasNameRange(const MatchFinder::MatchResult ) {
+static CharSourceRange
+getAliasNameRange(const MatchFinder::MatchResult ) {
   if (const auto *Using = Result.Nodes.getNodeAs("using")) {
 return CharSourceRange::getTokenRange(
 Using->getNameInfo().getSourceRange());
@@ -223,8 +225,6 @@
   Result.Nodes.getNodeAs("typeloc")->getSourceRange());
 }
 
-} // namespace
-
 void UpgradeGoogletestCaseCheck::check(const MatchFinder::MatchResult ) {
   llvm::StringRef ReplacementText;
   CharSourceRange ReplacementRange;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-06 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added a comment.

I think will be good idea to replace //upgrade// with //modernize// to be 
consistent with similar checks in other module.




Comment at: 
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:170
+
+llvm::StringRef getNewMethodName(llvm::StringRef CurrentName) {
+  std::pair ReplacementMap[] = {

Function should be static instead on placed in anonymous namespace. See LLVM 
Coding Guidelines. Same for other places.



Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:13
+#include "../ClangTidyCheck.h"
+
+#include 

Please remove unnecessary empty line.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst:13
+
+The affected APIs are :
+

Please remove space before colon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

2019-06-06 Thread Alex Strelnikov via Phabricator via cfe-commits 
astrelni created this revision.
astrelni added reviewers: alexfh, hokein, aaron.ballman, JonasToth, EricWF.
astrelni added a project: clang-tools-extra.
Herald added subscribers: llvm-commits, cfe-commits, xazax.hun, mgorny.
Herald added projects: clang, LLVM.

Introduce a new check to upgrade user code based on API changes in Googletest.

The check finds uses of old Googletest APIs with "case" in their name and 
replaces them with the new APIs named with "suite".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62977

Files:
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
  clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/Inputs/gtest/gtest.h
  clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/google/BUILD.gn
@@ -28,6 +28,7 @@
 "OverloadedUnaryAndCheck.cpp",
 "TodoCommentCheck.cpp",
 "UnnamedNamespaceInHeaderCheck.cpp",
+"UpgradeGoogletestCaseCheck.cpp",
 "UsingNamespaceDirectiveCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp
@@ -0,0 +1,991 @@
+// RUN: %check_clang_tidy %s google-upgrade-googletest-case %t -- -- -I%S/Inputs
+
+#include "gtest/gtest.h"
+
+// 
+// Macros
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: REGISTER_TYPED_TEST_SUITE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: INSTANTIATE_TYPED_TEST_SUITE_P(FooPrefix, FooTest, FooTypes);
+
+#ifdef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE TYPED_TEST_SUITE
+#endif
+
+#ifdef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define TYPED_TEST_CASE_P TYPED_TEST_SUITE_P
+#endif
+
+#ifdef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef REGISTER_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define REGISTER_TYPED_TEST_CASE_P REGISTER_TYPED_TEST_SUITE_P
+#endif
+
+#ifdef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:2: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#undef INSTANTIATE_TYPED_TEST_CASE_P
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: Googletest APIs named with 'case' are deprecated; use equivalent APIs named with 'suite'
+#define INSTANTIATE_TYPED_TEST_CASE_P INSTANTIATE_TYPED_TEST_SUITE_P
+#endif
+
+TYPED_TEST_CASE(FooTest, FooTypes);
+TYPED_TEST_CASE_P(FooTest);
+REGISTER_TYPED_TEST_CASE_P(FooTest, FooTestName);
+INSTANTIATE_TYPED_TEST_CASE_P(FooPrefix, FooTest, FooTypes);
+
+// 
+//