[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein closed this revision. hokein added a comment. I have committed the patch for you, https://reviews.llvm.org/rL340800. Comment at: test/clang-tidy/abseil-no-namespace.cpp:10 +#include "absl/external-file.h" +// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved +// for implementation of the Abseil library and should not be opened in user deannagarcia wrote: > hokein wrote: > > Does the test get passed on the first command `%check_clang_tidy %s > > abseil-no-namespace %t -- -- -I %S/Inputs`? The first command will suppress > > all warning in headers, I think it will fail? > The test passes, and I'm pretty sure it's because this is a CHECK not a > CHECK-MESSAGES Ah, I thought it was `CHECK-MESSAGES`, note that `CHECK` and `CHECK-MESSAGE` don't match multiple lines, the message should be one-line. I have fixed for you. https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia added inline comments. Comment at: test/clang-tidy/abseil-no-namespace.cpp:10 +#include "absl/external-file.h" +// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved +// for implementation of the Abseil library and should not be opened in user hokein wrote: > Does the test get passed on the first command `%check_clang_tidy %s > abseil-no-namespace %t -- -- -I %S/Inputs`? The first command will suppress > all warning in headers, I think it will fail? The test passes, and I'm pretty sure it's because this is a CHECK not a CHECK-MESSAGES https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 162417.
deannagarcia added a comment.
Rebased the patch
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilMatcher.h
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/Inputs/absl/external-file.h
test/clang-tidy/Inputs/absl/strings/internal-file.h
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+/// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+/// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in user
+// code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl'
+namespace std {
+int i = 5;
+}
+}
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -6,6 +6,7 @@
.. toctree::
abseil-duration-division
abseil-faster-strsplit-delimiter
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+Ensures code does not open ``namespace absl`` as that violates Abseil's
+compatibility guidelines. Code should not open ``namespace absl`` as that
+conflicts with Abseil's compatibility guidelines and may result in breakage.
+
+Any code that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines `_ for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -70,6 +70,12 @@
Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the
delimiter is a single character string literal and replaces with a character.
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Ensures code does not open ``namespace absl`` as that violates Abseil's
+ compatibility guidelines.
+
- New :doc:`readability-magic-numbers
` check.
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check ensures users don't open namespace absl, as that violates
+/// Abseil's compatibility guidelines.
+///
+/// For the user-facing documentation see:
+///
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Looks good. Comment at: test/clang-tidy/abseil-no-namespace.cpp:10 +#include "absl/external-file.h" +// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved +// for implementation of the Abseil library and should not be opened in user Does the test get passed on the first command `%check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs`? The first command will suppress all warning in headers, I think it will fail? https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 161939.
deannagarcia marked 2 inline comments as done.
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilMatcher.h
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/Inputs/absl/external-file.h
test/clang-tidy/Inputs/absl/strings/internal-file.h
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+/// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+/// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in user
+// code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl'
+namespace std {
+int i = 5;
+}
+}
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,6 +5,7 @@
.. toctree::
abseil-duration-division
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+Ensures code does not open ``namespace absl`` as that violates Abseil's
+compatibility guidelines. Code should not open ``namespace absl`` as that
+conflicts with Abseil's compatibility guidelines and may result in breakage.
+
+Any code that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines `_ for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -64,6 +64,12 @@
floating-point context, and recommends the use of a function that
returns a floating-point value.
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Ensures code does not open ``namespace absl`` as that violates Abseil's
+ compatibility guidelines.
+
- New :doc:`readability-magic-numbers
` check.
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check ensures users don't open namespace absl, as that violates
+/// Abseil's compatibility guidelines.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-namespace.html
+class NoNamespaceCheck : public
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:60 +- New :doc:`abseil-no-namespace + ` check. Please sort new checks list alphabetically. Comment at: docs/ReleaseNotes.rst:79 +>>> .r340314 Improvements to include-fixer Please remove merge conflict residual. https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 161827.
deannagarcia marked 11 inline comments as done.
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilMatcher.h
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/Inputs/absl/external-file.h
test/clang-tidy/Inputs/absl/strings/internal-file.h
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+/// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+/// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in user
+// code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl'
+namespace std {
+int i = 5;
+}
+}
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -5,6 +5,7 @@
.. toctree::
abseil-duration-division
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+Ensures code does not open ``namespace absl`` as that violates Abseil's
+compatibility guidelines. Code should not open ``namespace absl`` as that
+conflicts with Abseil's compatibility guidelines and may result in breakage.
+
+Any code that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines `_ for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
Improvements to clang-tidy
--
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Ensures code does not open ``namespace absl`` as that violates Abseil's
+ compatibility guidelines.
+
- New :doc:`abseil-duration-division
` check.
@@ -70,6 +76,7 @@
Detects usage of magic numbers, numbers that are used as literals instead of
introduced via constants or symbols.
+>>> .r340314
Improvements to include-fixer
-
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check ensures users don't open namespace absl, as that violates
+/// Abseil's compatibility
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
Eugene.Zelenko added a comment. Please rebase from trunk. Comment at: docs/ReleaseNotes.rst:63 + + Checks to ensure code does not open `namespace absl` as that + violates Abseil's compatibility guidelines. Just Ensures. Please use `` for language constructs. Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:6 + +This check gives users a warning if they attempt to open ``namespace absl``. +Users should not open ``namespace absl`` as that conflicts with abseil's Please synchronize first sentence with Release Notes. Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:7 +This check gives users a warning if they attempt to open ``namespace absl``. +Users should not open ``namespace absl`` as that conflicts with abseil's +compatibility guidelines and may result in breakage. Abseil Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:10 + +Any user that uses: + Code instead of user, please. Comment at: docs/clang-tidy/checks/abseil-no-namespace.rst:20 + +See `the full Abseil compatibility guidelines `_ for more information. Please remove space after https: https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein added a comment.
Thanks for the updates. Looks mostly good, a few nits.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+
+AST_POLYMORPHIC_MATCHER(isExpansionInAbseilHeader,
+AST_POLYMORPHIC_SUPPORTED_TYPES(
nit: this matcher name now should be `isInAbseilFile`.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:26
+ Finder->addMatcher(
+ namespaceDecl(hasName("absl"), unless(isExpansionInAbseilHeader()))
+ .bind("abslNamespace"),
Does the `absl` namespace is always the top level namespace? If so, I think it
would be safer to use qualified name "::absl" here.
Comment at: clang-tidy/abseil/NoNamespaceCheck.h:19
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
nit: test => check.
Comment at: clang-tidy/abseil/NoNamespaceCheck.h:20
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
nit: our = abseil?
Comment at: test/clang-tidy/abseil-no-namespace.cpp:22
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl'
+
nit: put it immediately after the above `name absl {`.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hugoeg added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
deannagarcia wrote:
> deannagarcia wrote:
> > lebedev.ri wrote:
> > > hokein wrote:
> > > > lebedev.ri wrote:
> > > > > hokein wrote:
> > > > > > The regex seems incomplete, we are missing `algorithm`, `time`
> > > > > > subdirectory.
> > > > > So what happens if open the namespace in a file that is located in my
> > > > > personal `absl/base` directory?
> > > > > It will be false-negatively not reported?
> > > > Yes, I'd say this is a known limitation.
> > > Similarly, the check will break (start producing false-positives) as soon
> > > as a new directory is added to abseil proper.
> > > I don't have any reliable idea on how to do this better, but the current
> > > solution seems rather suboptimal..
> > We are aware that there will be a false-negative if code opens namespace in
> > the absl directories, however we think that is pretty rare and that users
> > shouldn't be doing that anyway. No matter how we do this there will be a
> > way for users to circumvent the check, and we will allow those users to do
> > it because it will be their code that breaks in the end.
> The false-positive with new directories is something we thought about, but
> new directories aren't added to absl very often so we thought it wouldn't be
> too hard to add them to this regex when they are.
Members of the Abseil team can also update this check as new directories are
release
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
lebedev.ri wrote:
> hokein wrote:
> > lebedev.ri wrote:
> > > hokein wrote:
> > > > The regex seems incomplete, we are missing `algorithm`, `time`
> > > > subdirectory.
> > > So what happens if open the namespace in a file that is located in my
> > > personal `absl/base` directory?
> > > It will be false-negatively not reported?
> > Yes, I'd say this is a known limitation.
> Similarly, the check will break (start producing false-positives) as soon as
> a new directory is added to abseil proper.
> I don't have any reliable idea on how to do this better, but the current
> solution seems rather suboptimal..
We are aware that there will be a false-negative if code opens namespace in the
absl directories, however we think that is pretty rare and that users shouldn't
be doing that anyway. No matter how we do this there will be a way for users to
circumvent the check, and we will allow those users to do it because it will be
their code that breaks in the end.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
deannagarcia wrote:
> lebedev.ri wrote:
> > hokein wrote:
> > > lebedev.ri wrote:
> > > > hokein wrote:
> > > > > The regex seems incomplete, we are missing `algorithm`, `time`
> > > > > subdirectory.
> > > > So what happens if open the namespace in a file that is located in my
> > > > personal `absl/base` directory?
> > > > It will be false-negatively not reported?
> > > Yes, I'd say this is a known limitation.
> > Similarly, the check will break (start producing false-positives) as soon
> > as a new directory is added to abseil proper.
> > I don't have any reliable idea on how to do this better, but the current
> > solution seems rather suboptimal..
> We are aware that there will be a false-negative if code opens namespace in
> the absl directories, however we think that is pretty rare and that users
> shouldn't be doing that anyway. No matter how we do this there will be a way
> for users to circumvent the check, and we will allow those users to do it
> because it will be their code that breaks in the end.
The false-positive with new directories is something we thought about, but new
directories aren't added to absl very often so we thought it wouldn't be too
hard to add them to this regex when they are.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 161720.
deannagarcia marked 12 inline comments as done.
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilMatcher.h
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/Inputs/absl/external-file.h
test/clang-tidy/Inputs/absl/strings/internal-file.h
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,26 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in
+// user code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+int i = 5;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl'
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
=
.. toctree::
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+This check gives users a warning if they attempt to open ``namespace absl``.
+Users should not open ``namespace absl`` as that conflicts with abseil's
+compatibility guidelines and may result in breakage.
+
+Any user that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines `_ for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,7 +57,11 @@
Improvements to clang-tidy
--
-The improvements are...
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Checks to ensure code does not open `namespace absl` as that
+ violates Abseil's compatibility guidelines.
Improvements to include-fixer
-
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-namespace.html
+class NoNamespaceCheck : public ClangTidyCheck {
+public:
+ NoNamespaceCheck(StringRef Name,
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
lebedev.ri added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
hokein wrote:
> lebedev.ri wrote:
> > hokein wrote:
> > > The regex seems incomplete, we are missing `algorithm`, `time`
> > > subdirectory.
> > So what happens if open the namespace in a file that is located in my
> > personal `absl/base` directory?
> > It will be false-negatively not reported?
> Yes, I'd say this is a known limitation.
Similarly, the check will break (start producing false-positives) as soon as a
new directory is added to abseil proper.
I don't have any reliable idea on how to do this better, but the current
solution seems rather suboptimal..
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
lebedev.ri wrote:
> hokein wrote:
> > The regex seems incomplete, we are missing `algorithm`, `time`
> > subdirectory.
> So what happens if open the namespace in a file that is located in my
> personal `absl/base` directory?
> It will be false-negatively not reported?
Yes, I'd say this is a known limitation.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
lebedev.ri added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
hokein wrote:
> The regex seems incomplete, we are missing `algorithm`, `time` subdirectory.
So what happens if open the namespace in a file that is located in my personal
`absl/base` directory?
It will be false-negatively not reported?
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein added inline comments.
Comment at: clang-tidy/abseil/AbseilMatcher.h:16
+
+/// Matches AST nodes that were expanded within abseil-header-files.
+AST_POLYMORPHIC_MATCHER(isExpansionNotInAbseilHeader,
nit: We need proper documentation for this matcher, since it is exposed to
users.
Comment at: clang-tidy/abseil/AbseilMatcher.h:17
+/// Matches AST nodes that were expanded within abseil-header-files.
+AST_POLYMORPHIC_MATCHER(isExpansionNotInAbseilHeader,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc,
- using negative for AST matcher seems uncommon in ASTMatchers, for negative we
have `unless`, so I'd suggest implementing `isInAbseilHeader`.
- it seems that this matcher is very similar to the matcher in
https://reviews.llvm.org/D50542. I think `isInAbseilFile` should also cover
your case here, and also ignore the warning if you run the check on abseil core
files.
Comment at: clang-tidy/abseil/AbseilMatcher.h:32
+ auto Filename = FileEntry->getName();
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
The regex seems incomplete, we are missing `algorithm`, `time` subdirectory.
Comment at: clang-tidy/abseil/AbseilMatcher.h:33
+ llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|"
+ "synchronization|types|utiliy)");
+ return (!RE.match(Filename));
typo: `utility`
Comment at: clang-tidy/abseil/AbseilMatcher.h:34
+ "synchronization|types|utiliy)");
+ return (!RE.match(Filename));
+}
nit: remove the outer `()`.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
hokein wrote:
> JonasToth wrote:
> > hugoeg wrote:
> > > deannagarcia wrote:
> > > > aaron.ballman wrote:
> > > > > hokein wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I think this needs a `not(isExpansionInSystemHeader())` in there,
> > > > > > > as this is going to trigger on code that includes a header file
> > > > > > > using an `absl` namespace. If I'm incorrect and users typically
> > > > > > > include abseil as something other than system includes, you'll
> > > > > > > have to find a different way to solve this.
> > > > > > Yeah, we have discussed this issue internally. abseil-user code
> > > > > > usually includes abseil header like `#include
> > > > > > "whatever/abseil/base/optimization.h"`, so
> > > > > > `IsExpansionInSystemHeader` doesn't work for most cases.
> > > > > >
> > > > > > We need a way to filter out all headers being a part of abseil
> > > > > > library. I think we can create a matcher
> > > > > > `InExpansionInAbseilHeader` -- basically using
> > > > > > `IsExpansionInFileMatching` with a regex expression that matches
> > > > > > typical abseil include path. What do you think?
> > > > > >
> > > > > > And we'll have more abseil checks (e.g. `abseil-no-internal-deps`)
> > > > > > that use `InExpansionInAbseilHeader`, we should put it to a common
> > > > > > header.
> > > > > I think that is a sensible approach.
> > > > We will begin working on this, I think it will first be implemented in
> > > > abseil-no-internal-deps but once it looks good I will add it to this
> > > > patch.
> > > I'm going to go ahead a implement this in ASTMatchers.h and include it on
> > > the patch for **abseil-no-internal-dep**s
> > In principle it is ok, but I don't think ASTMatchers.h is the correct
> > place, because it is only of use to clang-tidy.
> >
> > There is a `util` directory in clang-tidy for this kind of stuff. Defining
> > you own matchers works the same as in ASTMatchers, you can grep through
> > clang-tidy checks (e.g. `AST_MATCHER`) to have some examples of private
> > matchers.
> `ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers.
> We have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific
> matchers. I'm not sure whether it is reasonable to put abseil-specific
> matchers there. Maybe we create a `AbseilMatcher.h` file in
> `clang-tidy/abseil` directory?
I put it in clang-tidy/abseil for now but I can definitely move it to
clang-tidy/utils/ if you would prefer that
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 161526.
deannagarcia edited the summary of this revision.
deannagarcia added a comment.
This revision includes a matcher so that the warning does not trigger on
internal Abseil files.
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilMatcher.h
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/Inputs/absl/external-file.h
test/clang-tidy/Inputs/absl/strings/internal-file.h
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,26 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in
+// user code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+int i = 5;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl'
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: test/clang-tidy/Inputs/absl/strings/internal-file.h
===
--- test/clang-tidy/Inputs/absl/strings/internal-file.h
+++ test/clang-tidy/Inputs/absl/strings/internal-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: test/clang-tidy/Inputs/absl/external-file.h
===
--- test/clang-tidy/Inputs/absl/external-file.h
+++ test/clang-tidy/Inputs/absl/external-file.h
@@ -0,0 +1 @@
+namespace absl {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
=
.. toctree::
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+This check gives users a warning if they attempt to open ``namespace absl``.
+Users should not open ``namespace absl`` as that conflicts with abseil's
+compatibility guidelines and may result in breakage.
+
+Any user that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines `_ for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,7 +57,11 @@
Improvements to clang-tidy
--
-The improvements are...
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Checks to ensure code does not open `namespace absl` as that
+ violates Abseil's compatibility guidelines.
Improvements to include-fixer
-
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
+/// For the user-facing documentation see:
+///
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
JonasToth added a comment. That sounds good as well, just not in clang and best in `clang-tidy/utils` > `ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. > We have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific > matchers. I'm not sure whether it is reasonable to put abseil-specific > matchers there. Maybe we create a `AbseilMatcher.h` file in > `clang-tidy/abseil` directory? > > https://reviews.llvm.org/D50580 https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
JonasToth wrote:
> hugoeg wrote:
> > deannagarcia wrote:
> > > aaron.ballman wrote:
> > > > hokein wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this needs a `not(isExpansionInSystemHeader())` in there,
> > > > > > as this is going to trigger on code that includes a header file
> > > > > > using an `absl` namespace. If I'm incorrect and users typically
> > > > > > include abseil as something other than system includes, you'll have
> > > > > > to find a different way to solve this.
> > > > > Yeah, we have discussed this issue internally. abseil-user code
> > > > > usually includes abseil header like `#include
> > > > > "whatever/abseil/base/optimization.h"`, so
> > > > > `IsExpansionInSystemHeader` doesn't work for most cases.
> > > > >
> > > > > We need a way to filter out all headers being a part of abseil
> > > > > library. I think we can create a matcher `InExpansionInAbseilHeader`
> > > > > -- basically using `IsExpansionInFileMatching` with a regex
> > > > > expression that matches typical abseil include path. What do you
> > > > > think?
> > > > >
> > > > > And we'll have more abseil checks (e.g. `abseil-no-internal-deps`)
> > > > > that use `InExpansionInAbseilHeader`, we should put it to a common
> > > > > header.
> > > > I think that is a sensible approach.
> > > We will begin working on this, I think it will first be implemented in
> > > abseil-no-internal-deps but once it looks good I will add it to this
> > > patch.
> > I'm going to go ahead a implement this in ASTMatchers.h and include it on
> > the patch for **abseil-no-internal-dep**s
> In principle it is ok, but I don't think ASTMatchers.h is the correct place,
> because it is only of use to clang-tidy.
>
> There is a `util` directory in clang-tidy for this kind of stuff. Defining
> you own matchers works the same as in ASTMatchers, you can grep through
> clang-tidy checks (e.g. `AST_MATCHER`) to have some examples of private
> matchers.
`ASTMatchers.h` is not a reasonable place to put `asseil`-specific matchers. We
have `clang-tidy/utils/Matchers.h` for putting clang-tidy specific matchers.
I'm not sure whether it is reasonable to put abseil-specific matchers there.
Maybe we create a `AbseilMatcher.h` file in `clang-tidy/abseil` directory?
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
JonasToth added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
hugoeg wrote:
> deannagarcia wrote:
> > aaron.ballman wrote:
> > > hokein wrote:
> > > > aaron.ballman wrote:
> > > > > I think this needs a `not(isExpansionInSystemHeader())` in there, as
> > > > > this is going to trigger on code that includes a header file using an
> > > > > `absl` namespace. If I'm incorrect and users typically include abseil
> > > > > as something other than system includes, you'll have to find a
> > > > > different way to solve this.
> > > > Yeah, we have discussed this issue internally. abseil-user code
> > > > usually includes abseil header like `#include
> > > > "whatever/abseil/base/optimization.h"`, so `IsExpansionInSystemHeader`
> > > > doesn't work for most cases.
> > > >
> > > > We need a way to filter out all headers being a part of abseil library.
> > > > I think we can create a matcher `InExpansionInAbseilHeader` --
> > > > basically using `IsExpansionInFileMatching` with a regex expression
> > > > that matches typical abseil include path. What do you think?
> > > >
> > > > And we'll have more abseil checks (e.g. `abseil-no-internal-deps`)
> > > > that use `InExpansionInAbseilHeader`, we should put it to a common
> > > > header.
> > > I think that is a sensible approach.
> > We will begin working on this, I think it will first be implemented in
> > abseil-no-internal-deps but once it looks good I will add it to this patch.
> I'm going to go ahead a implement this in ASTMatchers.h and include it on the
> patch for **abseil-no-internal-dep**s
In principle it is ok, but I don't think ASTMatchers.h is the correct place,
because it is only of use to clang-tidy.
There is a `util` directory in clang-tidy for this kind of stuff. Defining you
own matchers works the same as in ASTMatchers, you can grep through clang-tidy
checks (e.g. `AST_MATCHER`) to have some examples of private matchers.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hugoeg added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
deannagarcia wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > I think this needs a `not(isExpansionInSystemHeader())` in there, as
> > > > this is going to trigger on code that includes a header file using an
> > > > `absl` namespace. If I'm incorrect and users typically include abseil
> > > > as something other than system includes, you'll have to find a
> > > > different way to solve this.
> > > Yeah, we have discussed this issue internally. abseil-user code usually
> > > includes abseil header like `#include
> > > "whatever/abseil/base/optimization.h"`, so `IsExpansionInSystemHeader`
> > > doesn't work for most cases.
> > >
> > > We need a way to filter out all headers being a part of abseil library. I
> > > think we can create a matcher `InExpansionInAbseilHeader` -- basically
> > > using `IsExpansionInFileMatching` with a regex expression that matches
> > > typical abseil include path. What do you think?
> > >
> > > And we'll have more abseil checks (e.g. `abseil-no-internal-deps`) that
> > > use `InExpansionInAbseilHeader`, we should put it to a common header.
> > I think that is a sensible approach.
> We will begin working on this, I think it will first be implemented in
> abseil-no-internal-deps but once it looks good I will add it to this patch.
I'm going to go ahead a implement this in ASTMatchers.h and include it on the
patch for **abseil-no-internal-dep**s
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
aaron.ballman wrote:
> hokein wrote:
> > aaron.ballman wrote:
> > > I think this needs a `not(isExpansionInSystemHeader())` in there, as this
> > > is going to trigger on code that includes a header file using an `absl`
> > > namespace. If I'm incorrect and users typically include abseil as
> > > something other than system includes, you'll have to find a different way
> > > to solve this.
> > Yeah, we have discussed this issue internally. abseil-user code usually
> > includes abseil header like `#include
> > "whatever/abseil/base/optimization.h"`, so `IsExpansionInSystemHeader`
> > doesn't work for most cases.
> >
> > We need a way to filter out all headers being a part of abseil library. I
> > think we can create a matcher `InExpansionInAbseilHeader` -- basically
> > using `IsExpansionInFileMatching` with a regex expression that matches
> > typical abseil include path. What do you think?
> >
> > And we'll have more abseil checks (e.g. `abseil-no-internal-deps`) that
> > use `InExpansionInAbseilHeader`, we should put it to a common header.
> I think that is a sensible approach.
We will begin working on this, I think it will first be implemented in
abseil-no-internal-deps but once it looks good I will add it to this patch.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia updated this revision to Diff 160399.
deannagarcia marked 7 inline comments as done.
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-no-namespace.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+int i = 5;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
=
.. toctree::
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/clang-tidy/checks/abseil-no-namespace.rst
===
--- docs/clang-tidy/checks/abseil-no-namespace.rst
+++ docs/clang-tidy/checks/abseil-no-namespace.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===
+
+This check gives users a warning if they attempt to open
+``namespace absl``. Users should not open ``namespace absl``
+as that conflicts with abseil's compatibility guidelines and
+may result in breakage.
+
+Any user that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+ ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines ` for more information.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,7 +57,11 @@
Improvements to clang-tidy
--
-The improvements are...
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Checks to ensure code does not open `namespace absl` as that
+ violates Abseil's compatibility guidelines.
Improvements to include-fixer
-
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,36 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-namespace.html
+class NoNamespaceCheck : public ClangTidyCheck {
+public:
+ NoNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
Index: clang-tidy/abseil/NoNamespaceCheck.cpp
===
--- clang-tidy/abseil/NoNamespaceCheck.cpp
+++ clang-tidy/abseil/NoNamespaceCheck.cpp
@@ -0,0 +1,39 @@
+//===--- NoNamespaceCheck.cpp - clang-tidy-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "NoNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
hokein wrote:
> aaron.ballman wrote:
> > I think this needs a `not(isExpansionInSystemHeader())` in there, as this
> > is going to trigger on code that includes a header file using an `absl`
> > namespace. If I'm incorrect and users typically include abseil as something
> > other than system includes, you'll have to find a different way to solve
> > this.
> Yeah, we have discussed this issue internally. abseil-user code usually
> includes abseil header like `#include "whatever/abseil/base/optimization.h"`,
> so `IsExpansionInSystemHeader` doesn't work for most cases.
>
> We need a way to filter out all headers being a part of abseil library. I
> think we can create a matcher `InExpansionInAbseilHeader` -- basically using
> `IsExpansionInFileMatching` with a regex expression that matches typical
> abseil include path. What do you think?
>
> And we'll have more abseil checks (e.g. `abseil-no-internal-deps`) that use
> `InExpansionInAbseilHeader`, we should put it to a common header.
I think that is a sensible approach.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
hokein added a comment.
The check is missing its document, please add one in `docs/clang-tidy/checks/`.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
aaron.ballman wrote:
> I think this needs a `not(isExpansionInSystemHeader())` in there, as this is
> going to trigger on code that includes a header file using an `absl`
> namespace. If I'm incorrect and users typically include abseil as something
> other than system includes, you'll have to find a different way to solve this.
Yeah, we have discussed this issue internally. abseil-user code usually
includes abseil header like `#include "whatever/abseil/base/optimization.h"`,
so `IsExpansionInSystemHeader` doesn't work for most cases.
We need a way to filter out all headers being a part of abseil library. I think
we can create a matcher `InExpansionInAbseilHeader` -- basically using
`IsExpansionInFileMatching` with a regex expression that matches typical abseil
include path. What do you think?
And we'll have more abseil checks (e.g. `abseil-no-internal-deps`) that use
`InExpansionInAbseilHeader`, we should put it to a common header.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
lebedev.ri added a comment. > We are aware that this test will cause warnings on users code through their > dependencies on abseil. > However, from what we know it seems like these warnings are normally > suppressed. > If anyone has a good idea on how to avoid this/has insight on whether this > will be a problem for the average user, please let me know! Suppressed how? I think this needs tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
I think this needs a `not(isExpansionInSystemHeader())` in there, as this is
going to trigger on code that includes a header file using an `absl` namespace.
If I'm incorrect and users typically include abseil as something other than
system includes, you'll have to find a different way to solve this.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:32
+ "namespace 'absl' is reserved for implementation of the Abseil library "
+ "and should not be opened in the user code");
+}
in the user code -> in user code
Does Abseil prohibit the user from specializing templates in the `absl`
namespace with user-defined types?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
JonasToth added a comment.
Could it happen that some template specializations or so need to land in `absl`?
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:28
+void NoNamespaceCheck::check(const MatchFinder::MatchResult ) {
+ const auto *decl = Result.Nodes.getNodeAs("absl_namespace");
+
Please follow LLVM naming convention (Capitalize, and Decl should crash with a
type)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
Eugene.Zelenko added a comment.
Check documentation is missing.
Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:21
+void NoNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus) return;
+
Please place return in separate line.
Comment at: clang-tidy/abseil/NoNamespaceCheck.h:25
+class NoNamespaceCheck : public ClangTidyCheck {
+ public:
+ NoNamespaceCheck(StringRef Name, ClangTidyContext *Context)
Please run Clang-format.
Comment at: docs/ReleaseNotes.rst:60
The improvements are...
Please remove placeholder.
Comment at: docs/ReleaseNotes.rst:65
+
+ Checks to ensure user did not open namespace absl as that
+ violates abseil's compatibility guidelines.
Please reformulate to refer to code, not user. Please enclose absl in ``.
Comment at: docs/ReleaseNotes.rst:66
+ Checks to ensure user did not open namespace absl as that
+ violates abseil's compatibility guidelines.
+
abseil ->Abseil.
https://reviews.llvm.org/D50580
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50580: [clang-tidy] Abseil: no namespace check
deannagarcia created this revision.
deannagarcia added reviewers: alexfh, hokein.
deannagarcia added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.
This check ensures that users of Abseil do not open namespace absl in their
code, as that violates our compatibility guidelines.
We are aware that this test will cause warnings on users code through their
dependencies on abseil. However, from what we know it seems like these warnings
are normally suppressed. If anyone has a good idea on how to avoid this/has
insight on whether this will be a problem for the average user, please let me
know!
https://reviews.llvm.org/D50580
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoNamespaceCheck.cpp
clang-tidy/abseil/NoNamespaceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-no-namespace.cpp
Index: test/clang-tidy/abseil-no-namespace.cpp
===
--- test/clang-tidy/abseil-no-namespace.cpp
+++ test/clang-tidy/abseil-no-namespace.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s abseil-no-namespace %t
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+int i = 5;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
=
.. toctree::
+ abseil-no-namespace
abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -59,6 +59,12 @@
The improvements are...
+- New :doc:`abseil-no-namespace
+ ` check.
+
+ Checks to ensure user did not open namespace absl as that
+ violates abseil's compatibility guidelines.
+
Improvements to include-fixer
-
Index: clang-tidy/abseil/NoNamespaceCheck.h
===
--- clang-tidy/abseil/NoNamespaceCheck.h
+++ clang-tidy/abseil/NoNamespaceCheck.h
@@ -0,0 +1,37 @@
+//===--- NoNamespaceCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-namespace.html
+class NoNamespaceCheck : public ClangTidyCheck {
+ public:
+ NoNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
Index: clang-tidy/abseil/NoNamespaceCheck.cpp
===
--- clang-tidy/abseil/NoNamespaceCheck.cpp
+++ clang-tidy/abseil/NoNamespaceCheck.cpp
@@ -0,0 +1,38 @@
+//===--- NoNamespaceCheck.cpp - clang-tidy-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "NoNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void NoNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus) return;
+
+ Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);
+}
+
+void NoNamespaceCheck::check(const MatchFinder::MatchResult ) {
+
