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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF closed this revision.
EricWF added a comment.

Committed in 020ed6713d889a95f8c98d7725c87b458d99f6b3 
.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM




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

EricWF wrote:
> rogeeff wrote:
> > 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.
> @lebedev.ri This also happened when the source location was inside a macro 
> expansion. Which is solved by getting the spelling loc, and there is a test 
> case that covers it.
> Newly added test cases were triggering the failure.

Thanks for the clarification!


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

[Eric Fiselier via Phabricator via cfe-commits]

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

LGTM.

The weird template test case might is derived from an old failure that I 
believe was addressed in Clang, but regression tests are fine by me.
The macro expansion fix is correct and correctly tested.




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

rogeeff wrote:
> 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.
@lebedev.ri This also happened when the source location was inside a macro 
expansion. Which is solved by getting the spelling loc, and there is a test 
case that covers it.


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

[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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?


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

[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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



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

Is there test coverage for this? When does/can this happen?


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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D72484#1817187 , @rogeeff wrote:

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


Reviewers should make comments and finally approve patch when everything would 
be fine. Please keep in mind that people may be busy in real life.


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

[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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mark fixed comments as done.


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

[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: [clang-tidy] Fix check for Abseil internal namespace access

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Should be change mentioned in Release Notes?




Comment at: 
clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:40
 
-  diag(InternalDependency->getBeginLoc(),
+  auto loc_at_fault =
+  Result.SourceManager->getSpellingLoc(InternalDependency->getBeginLoc());

Please don't use auto when type is not spelled in statement or iterator. Does 
variable name comply to LLVM Coding Guidelines?


Repository:
  rCTE Clang Tools Extra

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