[PATCH] D55346: [clang-tidy] check for using declaration scope and qualification

[Shyan Akmal via Phabricator via cfe-commits]

Naysh created this revision.
Naysh added reviewers: EricWF, JonasToth.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This patch adds four new, small checks corresponding to the guidelines in 
https://abseil.io/tips/119.

The checks are:

- Alias Free Headers: makes sure header files do not use convenience aliases
- Anonymous Enclosed Aliases: suggests that using declarations be moved to 
existing anonymous namespaces
- Qualified Aliases: suggests using declarations be fully qualified
- Safely Scoped: suggests using declarations be moved to innermost namespaces


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55346

Files:
  .gitignore
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.h
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp
  clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/QualifiedAliasesCheck.cpp
  clang-tidy/abseil/QualifiedAliasesCheck.h
  clang-tidy/abseil/SafelyScopedCheck.cpp
  clang-tidy/abseil/SafelyScopedCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-alias-free-headers.rst
  docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
  docs/clang-tidy/checks/abseil-qualified-aliases.rst
  docs/clang-tidy/checks/abseil-safely-scoped.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-alias-free-headers.hpp
  test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
  test/clang-tidy/abseil-qualified-aliases.cpp
  test/clang-tidy/abseil-safely-scoped.cpp
  test/clang-tidy/temporaries.cpp

Index: test/clang-tidy/temporaries.cpp
===
--- test/clang-tidy/temporaries.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// REQUIRES: static-analyzer
-// RUN: clang-tidy -checks='-*,clang-analyzer-core.NullDereference' %s -- | FileCheck %s
-
-struct NoReturnDtor {
-  ~NoReturnDtor() __attribute__((noreturn));
-};
-
-extern bool check(const NoReturnDtor &);
-
-// CHECK-NOT: warning
-void testNullPointerDereferencePositive() {
-  int *value = 0;
-  // CHECK: [[@LINE+1]]:10: warning: Dereference of null pointer (loaded from variable 'value') [clang-analyzer-core.NullDereference]
-  *value = 1;
-}
-
-// CHECK-NOT: warning
-void testNullPointerDereference() {
-  int *value = 0;
-  if (check(NoReturnDtor())) {
-// This unreachable code causes a warning if analysis of temporary
-// destructors is not enabled.
-*value = 1;
-  }
-}
Index: test/clang-tidy/abseil-safely-scoped.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-safely-scoped.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-safely-scoped %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the innermost scope. See: https://abseil.io/tips/119 [abseil-safely-scoped]
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
+
Index: test/clang-tidy/abseil-qualified-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-qualified-aliases.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-qualified-aliases %t
+
+namespace foo {
+  void f();
+  void correct();
+}
+
+namespace bar {
+  using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+  using ::foo::correct;
+}
+
+namespace outermost {
+  namespace middle {
+namespace innermost {
+
+  enum Color {Red, Blue, Yellow};
+
+} // namespace innermost
+
+using innermost::Color;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+
+  } // namespace middle
+} // namespace example
+
Index: test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the anonymous namespace. See: https://abseil.io/tips/119 [abseil-anonymous-enclosed-aliases]
+
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo
Index: test/clang-tidy/abseil-alias-free-headers.hpp
===
--- /dev/null
+++ test/clang-tidy/abseil-alias-free-h

[PATCH] D55346: [clang-tidy] check for using declaration qualification

[Shyan Akmal via Phabricator via cfe-commits]

Naysh updated this revision to Diff 177116.
Naysh retitled this revision from "[clang-tidy] check for using declaration 
scope and qualification" to "[clang-tidy] check for using declaration 
qualification".
Naysh edited the summary of this revision.
Naysh added a comment.

Based off the request to split this patch into four separate patches 
(corresponding to each of the introduced checks), I've updated the patch to 
only include the "Qualified Aliases" check.

This is a check motivated by the discussion in https://abseil.io/tips/119, and 
flags using declarations that aren't fully qualified.


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

https://reviews.llvm.org/D55346

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/QualifiedAliasesCheck.cpp
  clang-tidy/abseil/QualifiedAliasesCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-qualified-aliases.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-qualified-aliases.cpp

Index: test/clang-tidy/abseil-qualified-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-qualified-aliases.cpp
@@ -0,0 +1,26 @@
+// RUN: %check_clang_tidy %s abseil-qualified-aliases %t
+
+namespace foo {
+  void f();
+  void correct();
+}
+
+namespace bar {
+  using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+  using ::foo::correct;
+}
+
+namespace outermost {
+  namespace middle {
+namespace innermost {
+
+  enum Color {Red, Blue, Yellow};
+
+} // namespace innermost
+
+using innermost::Color;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+
+  } // namespace middle
+} // namespace example
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -9,9 +9,10 @@
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
+   abseil-qualified-aliases
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
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-qualified-aliases.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-qualified-aliases.rst
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - abseil-qualified-aliases
+
+abseil-qualified-aliases
+
+
+Detects using declarations that are not fully qualified, and suggests
+that the developer fully qualify those declarations.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+void f();
+void correct();
+  }
+
+  namespace bar {
+using foo::f; // The check produces a warning here.
+using ::foo::correct; // The check sees no issue here.
+  }
+
+See https://abseil.io/tips/119 for a more in depth justification of this
+check.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -98,6 +98,12 @@
   Ensures code does not open ``namespace absl`` as that violates Abseil's
   compatibility guidelines.
 
+- New :doc:`abseil-qualified-aliases
+  ` check.
+
+  Detects using declarations that are not fully qualified, and suggests
+  that the developer fully qualify those declarations.
+
 - New :doc:`abseil-redundant-strcat-calls
   ` check.
 
Index: clang-tidy/abseil/QualifiedAliasesCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/QualifiedAliasesCheck.h
@@ -0,0 +1,35 @@
+//===--- QualifiedAliasesCheck.h - clang-tidy ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file 

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Shyan Akmal via Phabricator via cfe-commits]

Naysh created this revision.
Naysh added reviewers: EricWF, JonasToth.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This patch adds an "Alias Free Headers" check, based off the guidelines at 
http://google.github.io/styleguide/cppguide.html#Aliases, to the abseil module.

The purpose of the check is to eliminate using declarations from header files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55410

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.cpp
  clang-tidy/abseil/AliasFreeHeadersCheck.h
  clang-tidy/abseil/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-alias-free-headers.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-alias-free-headers.hpp

Index: test/clang-tidy/abseil-alias-free-headers.hpp
===
--- /dev/null
+++ test/clang-tidy/abseil-alias-free-headers.hpp
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s abseil-alias-free-headers %t
+
+namespace foo {
+  void f();
+}
+
+using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: convenience aliases in header files are dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases [abseil-alias-free-headers]
+
+void other_function();
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,14 +4,15 @@
 =
 
 .. toctree::
+   abseil-alias-free-headers
abseil-duration-division
abseil-duration-factory-float
abseil-faster-strsplit-delimiter
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append
+   abseil-string-find-startswith
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-alias-free-headers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/abseil-alias-free-headers.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - abseil-alias-free-headers
+
+abseil-alias-free-headers
+=
+
+Flags using declarations in header files, and suggests that these aliases be removed.
+
+A using declaration placed in a header file forces users of that header file
+accept the specified alias. Because of this, using declarations should almost never
+be used in a header file
+
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases discusses this
+issue in more detail
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`abseil-alias-free-headers
+  ` check.
+
+  Flags using declarations in header files, and suggests that
+  these aliases be removed
+
 - New :doc:`abseil-duration-division
   ` check.
 
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AliasFreeHeadersCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
Index: clang-tidy/abseil/AliasFreeHeadersCheck.h
===
--- /dev/null
+++ clang-tidy/abseil/AliasFreeHeadersCheck.h
@@ -0,0 +1,35 @@
+//===--- AliasFreeHeadersCheck.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_ALIASFREEHEADERSCHECK_H
+#define LLVM_CL

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Shyan Akmal via Phabricator via cfe-commits]

Naysh added inline comments.



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:13
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases 
discusses this
+issue in more detail

Eugene.Zelenko wrote:
> Missing link to guidelines,
@Eugene.Zelenko: Sorry, I'm not sure what you mean. Could you clarify what 
guidelines you're referring to? 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55410



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

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

[Shyan Akmal via Phabricator via cfe-commits]

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



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:41
+// to the vector containing all candidate using declarations.
+if (AnonymousNamespaceDecl) {
+   diag(MatchedUsingDecl->getLocation(),

aaron.ballman wrote:
> I don't think this logic works in practice because there's no way to 
> determine that the anonymous namespace is even a candidate for putting the 
> using declaration into it. Consider a common scenario where there's an 
> anonymous namespace declared in a header file (like an STL header outside of 
> the user's control), and a using declaration inside of an implementation 
> file. Just because the STL declared an anonymous namespace doesn't mean that 
> the user could have put their using declaration in it.
If we altered the check to only apply to anonymous namespaces and using 
declarations at namespace scope (that is, we only suggest aliases be moved to 
anonymous namespaces when the unnamed namespace and alias are themselves inside 
some other namespace), would this issue be resolved? 


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

https://reviews.llvm.org/D55409



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

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

[Shyan Akmal via Phabricator via cfe-commits]

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



Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:13
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases 
discusses this
+issue in more detail

aaron.ballman wrote:
> Eugene.Zelenko wrote:
> > Naysh wrote:
> > > Eugene.Zelenko wrote:
> > > > Missing link to guidelines,
> > > @Eugene.Zelenko: Sorry, I'm not sure what you mean. Could you clarify 
> > > what guidelines you're referring to? 
> > Please see documentation in other using-related checks.
> I think he's talking about the fact that this is text and not a hyperlink. It 
> should look something more like
> ```
> The `Abseil Style Guide 
> `_ discusses this 
> issue in more detail.
> ```
Ah okay, that makes sense. Thanks for your help!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55410



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