from:"Alex Strelnikov via Phabricator via 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 &Node,
-   const MatchFinder::MatchResult &Result) {
+static bool isInInstantiation(const NodeType &Node,
+  const MatchFinder::MatchResult &Result) {
   return !match(isInTemplateInstantiation(), Node, *Result.Context).empty();
 }
 
 template 
-bool isInTemplate(const NodeType &Node,
-  const MatchFinder::MatchResult &Result) {
+static bool isInTemplate(const NodeType &Node,
+ const MatchFinder::MatchResult &Result) {
   internal::Matcher IsInsideTemplate =
   hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl(;
   return !match(IsInsideTemplate, Node, *Result.Context).empty();
 }
 
-bool derivedTypeHasReplacementMethod(const MatchFinder::MatchResult &Result,
- llvm::StringRef ReplacementMethod) {
+static bool
+derivedTypeHasReplacementMethod(const MatchFinder::MatchResult &Result,
+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 &Result) {
+static CharSourceRange
+getAliasNameRange(const MatchFinder::MatchResult &Result) {
   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 &Result) {
   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-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 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 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();
+  // 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 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: [[@LINE-1]]:

[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 void

[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 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-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 F

[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 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 FooTest::SetUpTest

[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-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);
+
+// 
+// testi

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-21 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 174931.

https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating-point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+These operators and factories will be changed to only accept arithmetic types to
+prevent unintended behavior. After these changes are released, passing an
+argument of class type will no longer compile, even if the type is implicitly
+convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating-point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,473 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(ConvertibleTo, ConvertibleTo);
+
+void arithmeticOperatorBasicPositive() {
+  absl::Duration d;
+  d *= ConvertibleTo();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning:

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-21 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni marked 5 inline comments as done.
astrelni added a comment.

Sorry for the long delay.

I've reworked the template instantiation stuff a little bit yet again. Going to 
still come back and comment with results of profiling but I think now this 
shouldn't be much slower than if the template instantiations were filtered out 
with the matchers.




Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:153
+  // required so we provide only a warning.
+  std::sort(MatchedTemplateLocations.begin(), MatchedTemplateLocations.end());
+  for (SourceLocation Loc : MatchedInstantiationLocations) {

alexfh wrote:
> MatchedTemplateLocations can be a std::set or a 
> std::unordered_set filled with SourceLocation::getRawEncoding(). 
> Same for MatchedInstantiationLocations, I suppose.
Ah thank you, I didn't look hard enough after not seeing a std::hash 
specialization for SourceLocation. In that case we don't need the 
onEndOfTranslationUnit or MatchedInstantiationLocations so those are now gone. 
I added a test case for when a template is used in source before the body since 
I wasn't 100% sure that the template would be traversed before the 
instantiation.


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'upgrade-duration-conversions'

2018-10-29 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni created this revision.
astrelni added reviewers: ahedberg, hokein.
astrelni added a project: clang-tools-extra.
Herald added subscribers: jfb, xazax.hun, mgorny.

Introduce a new check to upgrade user code based on upcoming API breaking 
changes to absl::Duration.

The check finds calls to arithmetic operators and factory functions for 
absl::Duration that rely on an implicit user defined conversion to int64_t. 
These cases will no longer compile after proposed changes are released. 
Suggested fixes explicitly cast the argument int64_t.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -10,8 +10,9 @@
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
+   abseil-str-cat-append
abseil-string-find-startswith
-   abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+The operators and factories will be updated to only accept arithmetic types to
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating point type.
+
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,357 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-29 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 171566.
astrelni added a comment.

Reply to comments: add check for language being c++, explicitly name type in 
declaration, alphabetize release notes.


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -10,8 +10,9 @@
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
+   abseil-str-cat-append
abseil-string-find-startswith
-   abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -151,6 +152,7 @@
hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) 
hicpp-static-assert (redirects to misc-static-assert) 
hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) 
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
hicpp-use-auto (redirects to modernize-use-auto) 
hicpp-use-emplace (redirects to modernize-use-emplace) 
hicpp-use-equals-default (redirects to modernize-use-equals-default) 
@@ -159,7 +161,6 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) 
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+The operators and factories will be updated to only accept arithmetic types to
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating point type.
+
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,357 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+co

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 171925.
astrelni marked 9 inline comments as done.
astrelni added a comment.

Thanks for the feedback so far.

Reply to review comments.

- Improve comments.
- Fix matcher to check for invalid source range.
- Improve test coverage for templates and macros.


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,42 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+The operators and factories will be updated to only accept arithmetic types to
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,400 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+void arithmeticOperatorBasicPositive() {
+  absl::D

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni marked an inline comment as done.
astrelni added inline comments.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:118
+AST_MATCHER_P(Expr, hasSourceRange, SourceRange, Range) {
+  return Node.getSourceRange() == Range;
+}

JonasToth wrote:
> What happens on invalid ranges? Are they considered equal, or is it forbidden 
> to pass them in?
Good point. I think this is a non-issue with the way the code is set up now, 
but better make it explicitly work :). We only care about valid source ranges. 
I've updated the name of the matcher and added a check here for the Node's 
source range and another in the matcher below to return false early.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:123
+  auto HasMatchingDependentDescendant = hasDescendant(
+  expr(hasSourceRange(Node.getSourceRange()), isInstantiationDependent()));
+  return expr(anyOf(hasAncestor(

JonasToth wrote:
> Please add a test-case where the `Node.getSourceRange()` is a macro, if not 
> already existing. I believe that is currently missing.
Yes, thank you, that was missing. Added a few macro + template interactions.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:158
+   *Result.Context)
+ .empty()) {
+  diag(ArgExpr->getBeginLoc(), Message);

JonasToth wrote:
> You could ellide these braces, but I feel that this matching code be merged 
> into one matcher with `equalsBoundNode()` (see ASTMatcher reference).
Started with removing braces.

Sorry I had a look at `equalsBoundNode()`, but couldn't see exactly what you 
meant. Could you please elaborate about the merging?



Comment at: test/clang-tidy/abseil-upgrade-duration-conversions.cpp:142
+
+template  void templateForOpsSpecialization(T) {}
+template <>

JonasToth wrote:
> what about non-type template parameters? Do they need consideration for the 
> check as well?
> If i am not mistaken floats are allowed in newwer standards as well.
IIUC non-type template parameters should be no different for this check. This 
particular case is to make sure explicit specializations are treated 
differently from instantiations and get fix-it hints. 


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 171973.
astrelni added a comment.

Combine template instantiation matching into the other matcher registration.


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,42 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+The operators and factories will be updated to only accept arithmetic types to
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,400 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+void arithmeticOperatorBasicPositive() {
+  absl::Duration d;
+  d *= ConvertibleTo();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: perform explicit cast on expression getting implicitly converte

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni marked 3 inline comments as done.
astrelni added inline comments.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:158
+   *Result.Context)
+ .empty()) {
+  diag(ArgExpr->getBeginLoc(), Message);

JonasToth wrote:
> astrelni wrote:
> > JonasToth wrote:
> > > You could ellide these braces, but I feel that this matching code be 
> > > merged into one matcher with `equalsBoundNode()` (see ASTMatcher 
> > > reference).
> > Started with removing braces.
> > 
> > Sorry I had a look at `equalsBoundNode()`, but couldn't see exactly what 
> > you meant. Could you please elaborate about the merging?
> i do not use that matcher on a daily basis, so take it with a grain of salt, 
> please :)
> 
> prototype:
> ```
> match(unless(allOf(isInTemplateInstantiation(equalsBoundNode("arg"))),
>  
> expr(isInstantiationOfDependentExpr(equalsBoundNode("call")
> ```
> You could do that matching in the `registerMatchers` even. The 
> `equalsBoundNode` is suitable to connect different parts of a complexer 
> matcher.
> If you want, you can explore it. Slight simplification would be desirable 
> here, but if it's not feasable the current form works too.
Got it work out in registerMatchers and without needing equalsBoundNode, thanks 
for prodding me in that direction. I tried it initially when writing the check 
and can't remember why I didn't get it to work last time.


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 172000.
astrelni marked 2 inline comments as done.
astrelni added a comment.

Reply to review comments:

- minor code reorder
- improve test coverage


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,42 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+The operators and factories will be updated to only accept arithmetic types to
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,451 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(ConvertibleTo, ConvertibleTo);
+
+void arithmeticOpera

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni added inline comments.



Comment at: test/clang-tidy/abseil-upgrade-duration-conversions.cpp:142
+
+template  void templateForOpsSpecialization(T) {}
+template <>

JonasToth wrote:
> astrelni wrote:
> > JonasToth wrote:
> > > what about non-type template parameters? Do they need consideration for 
> > > the check as well?
> > > If i am not mistaken floats are allowed in newwer standards as well.
> > IIUC non-type template parameters should be no different for this check. 
> > This particular case is to make sure explicit specializations are treated 
> > differently from instantiations and get fix-it hints. 
> Please test in that case that the behaviour is as expected.
Added some cases. I hope I interpreted correctly that "in that case" meant for 
template specializations / instantiations for non type template parameters.


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 172005.
astrelni marked 3 inline comments as done.
astrelni added a comment.

Reply to comments:

- Change diagnostic message
- Update documentation


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating-point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+These operators and factories will be changed to only accept arithmetic types to
+prevent unintended behavior. After these changes are released, passing an
+argument of class type will no longer compile, even if the type is implicitly
+convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating-point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,451 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(ConvertibleTo, Convertib

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni added inline comments.



Comment at: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst:22
+prevent unintended behavior. Passing an argument of class type will result in
+compile error, even if the type is implicitly convertible to an arithmetic 
type.
+

aaron.ballman wrote:
> compile error -> a diagnostic
Tried rewording for clarity, let me know what you think. It doesn't seem like 
diagnostic is what's needed here as its talking about the upcoming API changes 
to Abseil and not the affects of this check.


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-10-31 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni added inline comments.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:151-152
+  const auto *ArgExpr = Result.Nodes.getNodeAs("arg");
+  llvm::StringRef Message = "perform explicit cast on expression getting "
+"implicitly converted to int64_t";
+

aaron.ballman wrote:
> How about: `"implicit conversion to 'int64_t' is deprecated in this context; 
> use an explicit cast instead"`
Forgot to say thanks, this is much better :)


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-02 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 172402.
astrelni added a comment.
Herald added a subscriber: mgrang.

Updated filtering of template instantiations to not use potentially costly 
hasDescendent matcher.


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating-point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+These operators and factories will be changed to only accept arithmetic types to
+prevent unintended behavior. After these changes are released, passing an
+argument of class type will no longer compile, even if the type is implicitly
+convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating-point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,451 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(Conv

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-02 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni marked an inline comment as done.
astrelni added inline comments.



Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:33-36
+  anyOf(hasAncestor(
+functionTemplateDecl(HasMatchingDependentDescendant)),
+hasAncestor(
+classTemplateDecl(HasMatchingDependentDescendant

alexfh wrote:
> The hasAncestor and hasDescendant matchers frequently have non-trivial 
> performance implications. Especially the latter, especially when called on a 
> large number of nodes with a large number of transitive children (this 
> obviously depends on the code being analyzed). I don't know how likely is 
> this to cause problems here (the matcher seems to only be used when the check 
> is about to issue a diagnostic), but it's always good to be aware of the 
> possible issues.
> 
> Frequently there's a more efficient (and easier to understand) alternative. 
> For example, instead of trying to evaluate certain conditions while 
> traversing the code (which may require a large number of lookups into 
> relatively distant parts of the AST), it's sometimes more efficient to split 
> the analysis into two or more stages. During the AST traversal (i.e. when AST 
> matchers run) the check could collect some raw information about all 
> potentially problematic places in the code and then (for example, in 
> `onEndOfTranslationUnit`) analyze the collected information together in a 
> second stage. This works best if there's a way to arrange the data gathered 
> on the first pass such that the second pass can efficiently look up necessary 
> information.
Thanks, gave it a try, what do you think of this?


https://reviews.llvm.org/D53830



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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-15 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 174218.
astrelni marked an inline comment as done.
astrelni added a comment.

Fix to use `hasAnyName` everywhere


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating-point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+These operators and factories will be changed to only accept arithmetic types to
+prevent unintended behavior. After these changes are released, passing an
+argument of class type will no longer compile, even if the type is implicitly
+convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating-point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,451 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(ConvertibleTo, ConvertibleTo);
+
+void arithmeticOperatorBas

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

2018-11-15 Thread Alex Strelnikov via Phabricator via cfe-commits

astrelni updated this revision to Diff 174226.
astrelni added a comment.

Fix incorrect uploaded diff.


https://reviews.llvm.org/D53830

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
  clang-tidy/abseil/UpgradeDurationConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-upgrade-duration-conversions.cpp

Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -12,6 +12,7 @@
abseil-redundant-strcat-calls
abseil-string-find-startswith
abseil-str-cat-append
+   abseil-upgrade-duration-conversions
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
===
--- docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
+++ docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - abseil-upgrade-duration-conversions
+
+abseil-upgrade-duration-conversions
+===
+
+Finds calls to ``absl::Duration`` arithmetic operators and factories whose
+argument needs an explicit cast to continue compiling after upcoming API
+changes.
+
+The operators ``*=``, ``/=``, ``*``, and ``/`` for ``absl::Duration`` currently
+accept an argument of class type that is convertible to an arithmetic type. Such
+a call currently converts the value to an ``int64_t``, even in a case such as
+``std::atomic`` that would result in lossy conversion.
+
+Additionally, the ``absl::Duration`` factory functions (``absl::Hours``,
+``absl::Minutes``, etc) currently accept an ``int64_t`` or a floating-point
+type. Similar to the arithmetic operators, calls with an argument of class type
+that is convertible to an arithmetic type go through the ``int64_t`` path.
+
+These operators and factories will be changed to only accept arithmetic types to
+prevent unintended behavior. After these changes are released, passing an
+argument of class type will no longer compile, even if the type is implicitly
+convertible to an arithmetic type.
+
+Here are example fixes created by this check:
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(a);
+  d *= a;
+
+becomes
+
+.. code-block:: c++
+
+  std::atomic a;
+  absl::Duration d = absl::Milliseconds(static_cast(a));
+  d *= static_cast(a);
+
+Note that this check always adds a cast to ``int64_t`` in order to preserve the
+current behavior of user code. It is possible that this uncovers unintended
+behavior due to types implicitly convertible to a floating-point type.
Index: test/clang-tidy/abseil-upgrade-duration-conversions.cpp
===
--- test/clang-tidy/abseil-upgrade-duration-conversions.cpp
+++ test/clang-tidy/abseil-upgrade-duration-conversions.cpp
@@ -0,0 +1,451 @@
+// RUN: %check_clang_tidy %s abseil-upgrade-duration-conversions %t
+
+using int64_t = long long;
+
+// Partial implementation of relevant APIs from
+// https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
+namespace absl {
+
+class Duration {
+public:
+  Duration &operator*=(int64_t r);
+  Duration &operator*=(float r);
+  Duration &operator*=(double r);
+  template  Duration &operator*=(T r);
+
+  Duration &operator/=(int64_t r);
+  Duration &operator/=(float r);
+  Duration &operator/=(double r);
+  template  Duration &operator/=(T r);
+};
+
+template  Duration operator*(Duration lhs, T rhs);
+template  Duration operator*(T lhs, Duration rhs);
+template  Duration operator/(Duration lhs, T rhs);
+
+constexpr Duration Nanoseconds(int64_t n);
+constexpr Duration Microseconds(int64_t n);
+constexpr Duration Milliseconds(int64_t n);
+constexpr Duration Seconds(int64_t n);
+constexpr Duration Minutes(int64_t n);
+constexpr Duration Hours(int64_t n);
+
+template  struct EnableIfFloatImpl {};
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template <> struct EnableIfFloatImpl { typedef int Type; };
+template  using EnableIfFloat = typename EnableIfFloatImpl::Type;
+
+template  = 0> Duration Nanoseconds(T n);
+template  = 0> Duration Microseconds(T n);
+template  = 0> Duration Milliseconds(T n);
+template  = 0> Duration Seconds(T n);
+template  = 0> Duration Minutes(T n);
+template  = 0> Duration Hours(T n);
+
+} // namespace absl
+
+template  struct ConvertibleTo {
+  operator T() const;
+};
+
+template 
+ConvertibleTo operator+(ConvertibleTo, ConvertibleTo);
+
+template 
+ConvertibleTo operator*(ConvertibleTo, ConvertibleTo);
+
+void arithmeticOperatorBasicPositive() {
+  absl::Duration d;
+  d *= Conve

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

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

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

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

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

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

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

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

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

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

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

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

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

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

30 matches

Site Navigation

Mail list logo

Footer information