[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

2020-01-16 Thread Gennadiy Rozental via Phabricator via cfe-commits 
rogeeff marked 3 inline comments as done.
rogeeff added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:43-44
+
+  if (!LocAtFault.isValid())
+return;
+

aaron.ballman wrote:
> rogeeff wrote:
> > lebedev.ri wrote:
> > > Is there test coverage for this? When does/can this happen?
> > At the time when I wrote this internally the test cases in 
> > abseil-no-internal-dependencies.cpp were reproducing this failure. I'm not 
> > sure this is still the case. It is possible compiler was fixed since then.
> I am not certain I'm following along (sorry if I'm just being dense). Are you 
> saying that the existing test coverage in abseil-no-internal-dependencies.cpp 
> was failing for you internally, and that's the reason for this fix? Or are 
> you saying that the newly-added test cases in this patch were triggering this 
> failure?
Newly added test cases were triggering the failure.


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

https://reviews.llvm.org/D72484



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

[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

2020-01-13 Thread Gennadiy Rozental via Phabricator via cfe-commits 
rogeeff marked 2 inline comments as done.
rogeeff added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:43-44
+
+  if (!LocAtFault.isValid())
+return;
+

lebedev.ri wrote:
> Is there test coverage for this? When does/can this happen?
At the time when I wrote this internally the test cases in 
abseil-no-internal-dependencies.cpp were reproducing this failure. I'm not sure 
this is still the case. It is possible compiler was fixed since then.


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

https://reviews.llvm.org/D72484



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

[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

2020-01-13 Thread Gennadiy Rozental via Phabricator via cfe-commits 
rogeeff added a comment.

It is my first time submitting clang-tidy change. So I'm not sure of the 
procedure exactly. Do we wait for something from me?


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

https://reviews.llvm.org/D72484



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

[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

2020-01-09 Thread Gennadiy Rozental via Phabricator via cfe-commits 
rogeeff updated this revision to Diff 237225.

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

https://reviews.llvm.org/D72484

Files:
  clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
@@ -44,5 +44,18 @@
 void MacroUse() {
   USE_INTERNAL(Function); // no-warning
   USE_EXTERNAL(Function);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+  // CHECK-MESSAGES: :[[@LINE-5]]:25: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
 }
+
+class A : absl::container_internal::InternalStruct {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+
+template 
+class B : absl::container_internal::InternalTemplate {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+
+template  class C : absl::container_internal::InternalTemplate {
+public:
+  template  static C Make(U *p) { return C{}; }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:33: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
===
--- 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
@@ -15,6 +15,8 @@
 
 namespace container_internal {
 struct InternalStruct {};
+
+template  struct InternalTemplate {};
 } // namespace container_internal
 } // namespace absl
 
Index: clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
@@ -37,7 +37,13 @@
   const auto *InternalDependency =
   Result.Nodes.getNodeAs("InternalDep");
 
-  diag(InternalDependency->getBeginLoc(),
+  SourceLocation LocAtFault =
+  Result.SourceManager->getSpellingLoc(InternalDependency->getBeginLoc());
+
+  if (!LocAtFault.isValid())
+return;
+
+  diag(LocAtFault,
"do not reference any 'internal' namespaces; those implementation "
"details are reserved to Abseil");
 }


Index: clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
@@ -44,5 +44,18 @@
 void MacroUse() {
   USE_INTERNAL(Function); // no-warning
   USE_EXTERNAL(Function);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+  // CHECK-MESSAGES: :[[@LINE-5]]:25: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
 }
+
+class A : absl::container_internal::InternalStruct {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template 
+class B : absl::container_internal::InternalTemplate {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template  class C : absl::container_internal::InternalTemplate {
+public:
+  template  static C Make(U *p) { return C{}; }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:33: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
===
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
@@ -15,6 +15,8 @@
 
 namespace container_internal {
 struct InternalStruct {};
+
+template  struct InternalTemplate {};
 } // namespace container_internal
 } // namespace absl
 
Index:

[PATCH] D72484: Fix clang-tidy check for Abseil internal namespace access

2020-01-09 Thread Gennadiy Rozental via Phabricator via cfe-commits 
rogeeff created this revision.
Herald added a subscriber: mgehre.
Herald added a project: clang.
rogeeff added a reviewer: EricWF.

This change makes following modifications:

- If reference originated from macro expansion, we report location inside of 
the macro instead of location where macro is referenced
- If for any reason deduced location is not correct we silently ignore it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D72484

Files:
  clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
@@ -44,5 +44,18 @@
 void MacroUse() {
   USE_INTERNAL(Function); // no-warning
   USE_EXTERNAL(Function);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+  // CHECK-MESSAGES: :[[@LINE-5]]:25: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
 }
+
+class A : absl::container_internal::InternalStruct {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+
+template 
+class B : absl::container_internal::InternalTemplate {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
+
+template  class C : absl::container_internal::InternalTemplate {
+public:
+  template  static C Make(U *p) { return C{}; }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:33: warning: do not reference any 'internal' 
namespaces; those implementation details are reserved to Abseil
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
===
--- 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
@@ -15,6 +15,8 @@
 
 namespace container_internal {
 struct InternalStruct {};
+
+template  struct InternalTemplate {};
 } // namespace container_internal
 } // namespace absl
 
Index: clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
@@ -37,7 +37,13 @@
   const auto *InternalDependency =
   Result.Nodes.getNodeAs("InternalDep");
 
-  diag(InternalDependency->getBeginLoc(),
+  auto loc_at_fault =
+  Result.SourceManager->getSpellingLoc(InternalDependency->getBeginLoc());
+
+  if (!loc_at_fault.isValid())
+return;
+
+  diag(loc_at_fault,
"do not reference any 'internal' namespaces; those implementation "
"details are reserved to Abseil");
 }


Index: clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
@@ -44,5 +44,18 @@
 void MacroUse() {
   USE_INTERNAL(Function); // no-warning
   USE_EXTERNAL(Function);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+  // CHECK-MESSAGES: :[[@LINE-5]]:25: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
 }
+
+class A : absl::container_internal::InternalStruct {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template 
+class B : absl::container_internal::InternalTemplate {};
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+template  class C : absl::container_internal::InternalTemplate {
+public:
+  template  static C Make(U *p) { return C{}; }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:33: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
===
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
+++