[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

I've added the implicit access specifier check and run it on some bigger 
codebases. My findings are as below:

- Dolphin: 6 //triggers across// 856 //record types//
- OpenCV: 31 //triggers across// 3370 //record types//
- Inkscape: 39 //triggers across// 846 //record types//
- LLVM (Core + Clang + Clang Tools Extra): 128 //triggers across// 9214 
//record types//
- Blender: 948 //triggers across// 6264 //record types//

Since the check was triggered in every big project I've tested, I've decided to 
add an option (disabled by default) to enable the implicit access specifier 
check. It is included in my latest Diff, along with the rename to 
`readability-redundant-access-specifiers`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 181474.
m4tx marked an inline comment as not done.

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
  clang-tidy/readability/RedundantAccessSpecifiersCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
  
test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
  test/clang-tidy/readability-redundant-access-specifiers.cpp

Index: test/clang-tidy/readability-redundant-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-redundant-access-specifiers.CheckFirstDeclaration, value: 1}]}" --
+
+class FooPublic {
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int a;
+};
+
+struct StructPublic {
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int a;
+};
+
+union UnionPublic {
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int a;
+};
+
+class FooMacro {
+#if defined(ZZ)
+private:
+#endif
+  int a;
+};
+
+class ValidInnerStruct {
+  struct Inner {
+  private:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+MIXIN
+};
Index: docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
@@ -0,0 +1,4

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
m4tx added reviewers: alexfh, hokein.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

Add a bugprone check to clang-tidy that detects the usages of `delete this`. 
The warning is shown even when `delete this` is the last line in the method (as 
there is no guarantee that it won't be called from another method, or the 
pointer won't be used in any way afterwards).

https://bugs.llvm.org/show_bug.cgi?id=38741


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54262

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/DeleteThisCheck.cpp
  clang-tidy/bugprone/DeleteThisCheck.h
  docs/clang-tidy/checks/bugprone-delete-this.rst

Index: docs/clang-tidy/checks/bugprone-delete-this.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-delete-this.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - bugprone-delete-this
+
+bugprone-delete-this
+
+
+Detects the usages of ``delete this``.
+
+Said statement generates multiple problems, such as enforcing allocating the object via ``new``, or not allowing any
+usage of instance members (neither fields nor methods) after the execution of it, nor touching ``this`` pointer
+in any way.
+
+Example:
+
+.. code-block:: c++
+
+  class A {
+void foo() {
+  delete this;
+  // warning: usage of 'delete this' is suspicious [bugprone-delete-this]
+}
+  }
Index: clang-tidy/bugprone/DeleteThisCheck.h
===
--- /dev/null
+++ clang-tidy/bugprone/DeleteThisCheck.h
@@ -0,0 +1,35 @@
+//===--- DeleteThisCheck.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_BUGPRONE_DELETE_THIS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DELETE_THIS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Detects the usages of `delete this`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-delete-this.html
+class DeleteThisCheck : public ClangTidyCheck {
+public:
+  DeleteThisCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DELETE_THIS_H
Index: clang-tidy/bugprone/DeleteThisCheck.cpp
===
--- /dev/null
+++ clang-tidy/bugprone/DeleteThisCheck.cpp
@@ -0,0 +1,35 @@
+//===--- DeleteThisCheck.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 "DeleteThisCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void DeleteThisCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  cxxDeleteExpr(
+  has(ignoringParenImpCasts(cxxThisExpr(
+  .bind("DeleteThis"),
+  this);
+}
+
+void DeleteThisCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getNodeAs("DeleteThis");
+  diag(E->getBeginLoc(), "usage of 'delete this' is suspicious");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tidy/bugprone/CMakeLists.txt
+++ clang-tidy/bugprone/CMakeLists.txt
@@ -7,6 +7,7 @@
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
+  DeleteThisCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
Index: clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "BoolPointerImplicitConversionCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DeleteThisCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "Forw

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

After thinking about the possible use cases (and the difficulty of implementing 
heuristics for them) as well as fiddling with Clang Static Analyzer it seems 
that this patch can be discarded as the Analyzer already handles `delete this` 
pretty well. I've posted an update to the Bugzilla ticket.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54262



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 195408.
m4tx added a comment.
Herald added a project: clang.

Updated the diff for the latest (master) version, used GitHub monorepo for the 
changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55793

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
  
clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-redundant-access-specifiers.CheckFirstDeclaration, value: 1}]}" --
+
+class FooPublic {
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int a;
+};
+
+struct StructPublic {
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int a;
+};
+
+union UnionPublic {
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int a;
+};
+
+class FooMacro {
+#if defined(ZZ)
+private:
+#endif
+  int a;
+};
+
+class ValidInnerStruct {
+  struct Inner {
+  

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 195409.
m4tx added a comment.

Updated the backticks in the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55793

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
  
clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-redundant-access-specifiers.CheckFirstDeclaration, value: 1}]}" --
+
+class FooPublic {
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int a;
+};
+
+struct StructPublic {
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int a;
+};
+
+union UnionPublic {
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int a;
+};
+
+class FooMacro {
+#if defined(ZZ)
+private:
+#endif
+  int a;
+};
+
+class ValidInnerStruct {
+  struct Inner {
+  private:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class Va

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 197025.
m4tx added a comment.

Updated per @aaron.ballman comments


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

https://reviews.llvm.org/D55793

Files:
  0001-clang-tidy-Add-duplicated-access-specifier-readabili.patch
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
  
clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-redundant-access-specifiers.CheckFirstDeclaration, value: 1}]}" --
+
+class FooPublic {
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the implicit access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int a;
+};
+
+struct StructPublic {
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the implicit access specifier [readability-re

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx marked 15 inline comments as done.
m4tx added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:35-36
+  for (DeclContext::specific_decl_iterator
+   AS(MatchedDecl->decls_begin()),
+   ASEnd(MatchedDecl->decls_end());
+   AS != ASEnd; ++AS) {

aaron.ballman wrote:
> I have a slight preference for using assignment operators here rather than 
> explicit constructor calls.
This is not possible here as specific_decl_iterator has the copy constructor 
marked as `explicit`.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:54
+if (ASDecl->getAccess() == DefaultSpecifier) {
+  diag(ASDecl->getLocation(), "redundant access specifier")
+  << FixItHint::CreateRemoval(ASDecl->getSourceRange());

aaron.ballman wrote:
> This is a bit terse, how about: `redundant access specifier has the same 
> accessibility as the implicit access specifier`?
Sounds good, changed this in the latest revision.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:69
+
+  diag(ASDecl->getLocation(), "duplicated access specifier")
+  << FixItHint::CreateRemoval(ASDecl->getSourceRange());

aaron.ballman wrote:
> This is a bit terse, how about: `redundant access specifier has the same 
> accessibility as the previous access specifier`?
As above.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst:6
+
+Finds classes, structs and unions containing redundant member access 
specifiers.
+

aaron.ballman wrote:
> structs and unions -> structs, and unions
> 
> One thing the docs leave unclear is which access specifiers you're talking 
> about. Currently, the check only cares about the access specifiers for 
> fields, but it seems reasonable that the check could also be talking about 
> access specifiers on base class specifiers. e.g.,
> ```
> struct Base {
>   int a, b;
> };
> 
> class C : private Base { // The 'private' here is redundant.
> };
> ```
> You should probably clarify this in the docs. Implementing this functionality 
> may or may not be useful, but if you want to implement it, you could do it in 
> a follow-up patch.
This is actually included in the documentation (*member* access specifier), but 
I've added explicit "field and method" clarification.



Comment at: 
clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp:71-72
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier 
[readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}

aaron.ballman wrote:
> I think that diagnosing here is unfortunate. If the user defines `ZZ`, then 
> the access specifier is no longer redundant. However, it may not be easy for 
> you to handle this case when `ZZ` is not defined because the access specifier 
> will have been removed by the preprocessor.
I agree, but I think that we cannot do anything reasonable here. The code is 
removed by the preprocessor, and I believe the only behavior that would make 
sense would be to completely suppress the warnings if there is a preprocessor 
directive between the last access specifier and the current one. However, if 
I'm not mistaken, there is no way in clang-tidy to detect that.


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 197028.
m4tx marked 5 inline comments as done.
m4tx added a comment.

Remove the accidentally added patch file


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

https://reviews.llvm.org/D55793

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
  
clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s readability-redundant-access-specifiers %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-redundant-access-specifiers.CheckFirstDeclaration, value: 1}]}" --
+
+class FooPublic {
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the implicit access specifier [readability-redundant-access-specifiers]
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int a;
+};
+
+struct StructPublic {
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant access specifier has the same accessibility as the implicit access specifier [readability-redundant-access-specifi

[PATCH] D61256: [clang-format][docs] Fix the Google C++ and Chromium style guide URLs

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
m4tx added reviewers: djasper, MyDeveloperDay.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The Google C++ and Chromium style guides are broken in the clang-format docs. 
This patch updates them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61256

Files:
  clang/docs/ClangFormatStyleOptions.rst


Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -138,10 +138,10 @@
 `_
   * ``Google``
 A style complying with `Google's C++ style guide
-`_
+`_
   * ``Chromium``
 A style complying with `Chromium's style guide
-`_
+
`_
   * ``Mozilla``
 A style complying with `Mozilla's style guide
 `_


Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -138,10 +138,10 @@
 `_
   * ``Google``
 A style complying with `Google's C++ style guide
-`_
+`_
   * ``Chromium``
 A style complying with `Chromium's style guide
-`_
+`_
   * ``Mozilla``
 A style complying with `Mozilla's style guide
 `_
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

@JonasToth thanks for asking! Yes, since I do not have commit access, I need 
someone to do the commit for me.


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

https://reviews.llvm.org/D55793



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

[PATCH] D61256: [clang-format][docs] Fix the Google C++ and Chromium style guide URLs

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

@MyDeveloperDay thanks for accepting the revision! Since I do not have commit 
access, could you please make the commit for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61256



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

[PATCH] D63089: [clang] Warn on implicit boolean casts in more contexts (PR34180)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
m4tx added reviewers: lebedev.ri, rsmith, klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

  int x;
  return x = 5;

For a code like above, GCC produces a warning suggesting using parentheses 
about the assignment. This change makes clang produce similar warning to, 
suggesting either changing the operator to `==`, or wrap the expression inside 
parentheses.

This is based off D45401 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63089

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/parentheses.c
  clang/test/Sema/parentheses.cpp


Index: clang/test/Sema/parentheses.cpp
===
--- clang/test/Sema/parentheses.cpp
+++ clang/test/Sema/parentheses.cpp
@@ -215,3 +215,15 @@
 // fix-it:"{{.*}}":{[[@LINE-9]]:20-[[@LINE-9]]:20}:")"
   }
 }
+
+bool return_assign() {
+  int i;
+  return i = 4; // expected-warning {{assignment as a condition}} \
+// expected-note{{place parentheses around the assignment to 
silence this warning}} \
+// expected-note{{use '==' to turn this assignment into an 
equality comparison}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:12-[[@LINE-5]]:13}:"=="
+
+  return (i = 4);
+}
Index: clang/test/Sema/parentheses.c
===
--- clang/test/Sema/parentheses.c
+++ clang/test/Sema/parentheses.c
@@ -14,6 +14,18 @@
   if ((i = 4)) {}
 }
 
+_Bool return_assign(void) {
+  int i;
+  return i = 4; // expected-warning {{assignment as a condition}} \
+// expected-note{{place parentheses around the assignment to 
silence this warning}} \
+// expected-note{{use '==' to turn this assignment into an 
equality comparison}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:12-[[@LINE-5]]:13}:"=="
+
+  return (i = 4);
+}
+
 void bitwise_rel(unsigned i) {
   (void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} 
\
 // expected-note{{place parentheses around the '==' 
expression to silence this warning}} \
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -3926,6 +3926,11 @@
 FromType = From->getType();
   }
 
+  // Warn on assignments inside implicit casts (= instead of ==)
+  if (SCS.Second == ICK_Boolean_Conversion || FromType == Context.BoolTy) {
+DiagnoseAssignmentAsCondition(From->IgnoreImpCasts());
+  }
+
   // If we're converting to an atomic type, first convert to the corresponding
   // non-atomic type.
   QualType ToAtomicType;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16375,7 +16375,10 @@
 
 ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
bool IsConstexpr) {
-  DiagnoseAssignmentAsCondition(E);
+  // C++ implicit casts are checked inside PerformImplicitConversion
+  if (!getLangOpts().CPlusPlus) {
+DiagnoseAssignmentAsCondition(E);
+  }
   if (ParenExpr *parenE = dyn_cast(E))
 DiagnoseEqualityWithExtraParens(parenE);
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -9700,6 +9700,13 @@
   << FD << getLangOpts().CPlusPlus11;
 }
   }
+
+  // C++ implicit casts are checked inside PerformImplicitConversion
+  if (!getLangOpts().CPlusPlus) {
+if (ImplicitCastExpr *CastExpr = dyn_cast(RetValExp)) {
+  DiagnoseAssignmentAsCondition(CastExpr->IgnoreImpCasts());
+}
+  }
 }
 
 //===--- CHECK: Floating-Point comparisons (-Wfloat-equal) ---===//


Index: clang/test/Sema/parentheses.cpp
===
--- clang/test/Sema/parentheses.cpp
+++ clang/test/Sema/parentheses.cpp
@@ -215,3 +215,15 @@
 // fix-it:"{{.*}}":{[[@LINE-9]]:20-[[@LINE-9]]:20}:")"
   }
 }
+
+bool return_assign() {
+  int i;
+  return i = 4; // expected-warning {{assignment as a condition}} \
+// expected-note{{place parentheses around the assignment to silence this warning}} \
+// expected-note{{use '==' to turn this assignment into an equality comparison}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+  // CHECK: fix-it:"

[PATCH] D55790: [AST] Add accessSpecs() iterator range for CXXRecordDecl

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
Herald added a subscriber: cfe-commits.

Adds an iterator that allows to loop over C++ class/union/struct access 
specifier declarations.


Repository:
  rC Clang

https://reviews.llvm.org/D55790

Files:
  include/clang/AST/DeclCXX.h


Index: include/clang/AST/DeclCXX.h
===
--- include/clang/AST/DeclCXX.h
+++ include/clang/AST/DeclCXX.h
@@ -851,6 +851,22 @@
 return vbases_begin() + data().NumVBases;
   }
 
+  /// Iterator access to member access specifiers.
+  using accessspec_iterator = specific_decl_iterator;
+  using accessspec_range =
+  llvm::iterator_range>;
+
+  accessspec_range accessSpecs() const {
+return accessspec_range(accessspec_begin(), accessspec_end());
+  }
+  accessspec_iterator accessspec_begin() const {
+return accessspec_iterator(decl_iterator(FirstDecl));
+  }
+
+  accessspec_iterator accessspec_end() const {
+return accessspec_iterator(decl_iterator());
+  }
+
   /// Determine whether this class has any dependent base classes which
   /// are not the current instantiation.
   bool hasAnyDependentBases() const;


Index: include/clang/AST/DeclCXX.h
===
--- include/clang/AST/DeclCXX.h
+++ include/clang/AST/DeclCXX.h
@@ -851,6 +851,22 @@
 return vbases_begin() + data().NumVBases;
   }
 
+  /// Iterator access to member access specifiers.
+  using accessspec_iterator = specific_decl_iterator;
+  using accessspec_range =
+  llvm::iterator_range>;
+
+  accessspec_range accessSpecs() const {
+return accessspec_range(accessspec_begin(), accessspec_end());
+  }
+  accessspec_iterator accessspec_begin() const {
+return accessspec_iterator(decl_iterator(FirstDecl));
+  }
+
+  accessspec_iterator accessspec_end() const {
+return accessspec_iterator(decl_iterator());
+  }
+
   /// Determine whether this class has any dependent base classes which
   /// are not the current instantiation.
   bool hasAnyDependentBases() const;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx created this revision.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

This finds redundant access specifier declarations inside classes, structs, and 
unions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
  test/clang-tidy/readability-duplicated-access-specifiers.cpp

Index: test/clang-tidy/readability-duplicated-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-duplicated-access-specifiers.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+public:
+  int c;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+int x;
+int y;
+  public:
+int z;
+  protected:
+int a;
+  public:
+int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -238,6 +238,7 @@
readability-container-size-empty
readability-delete-null-pointer
readability-deleted-default
+   readability-duplicated-access-specifiers
readability-else-after-return
readability-function-size
readability-identifier-naming
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`abseil-duration-comparison
   ` check.
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
+#include "DuplicatedAccessSpecifiersCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
@@ -61,6 +62,8 @@
 "readability-delete-null-pointer");
 CheckFactories.registerCheck(
  

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 178539.
m4tx added a comment.

Use alphabetical order in ReleaseNotes.rst


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

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
  test/clang-tidy/readability-duplicated-access-specifiers.cpp

Index: test/clang-tidy/readability-duplicated-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-duplicated-access-specifiers.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+public:
+  int c;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+int x;
+int y;
+  public:
+int z;
+  protected:
+int a;
+  public:
+int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -238,6 +238,7 @@
readability-container-size-empty
readability-delete-null-pointer
readability-deleted-default
+   readability-duplicated-access-specifiers
readability-else-after-return
readability-function-size
readability-identifier-naming
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -192,6 +192,12 @@
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`readability-magic-numbers
   ` check.
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
+#include "DuplicatedAccessSpecifiersCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
@@ -61,6 +62,8 @@
 "readability-delete-null-pointer");
 CheckFactor

[PATCH] D55790: [AST] Add accessSpecs() iterator range for CXXRecordDecl

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

In D55790#1333627 , @riccibruno wrote:

> Do you really have to add an iterator for this particular thing ?
>  Why not just use `specific_decl_iterator` in your clang-tidy 
> checker ?


I don't need to - if this is the preferred way to accomplish this, I'm more 
than happy to modify the revisions. I just saw other range methods that are 
used only once  (e.g. `CXXRecordDecl::friends()`).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55790



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 178543.
m4tx added a comment.

Don't use `CXXRecordDecl::accessSpecs()`, use unique comments in tests.


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

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
  test/clang-tidy/readability-duplicated-access-specifiers.cpp

Index: test/clang-tidy/readability-duplicated-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-duplicated-access-specifiers.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+int x;
+int y;
+  public:
+int z;
+  protected:
+int a;
+  public:
+int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -238,6 +238,7 @@
readability-container-size-empty
readability-delete-null-pointer
readability-deleted-default
+   readability-duplicated-access-specifiers
readability-else-after-return
readability-function-size
readability-identifier-naming
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -192,6 +192,12 @@
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`readability-magic-numbers
   ` check.
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
+#include "DuplicatedAccessSpecifiersCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
@@ -61,6 +62,8 @@
 "readability-delete-nul

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 178777.
m4tx added a comment.

Fix variable names, add macro expansion checking as well as macro and inner 
class tests.


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

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
  test/clang-tidy/readability-duplicated-access-specifiers.cpp

Index: test/clang-tidy/readability-duplicated-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-duplicated-access-specifiers.cpp
@@ -0,0 +1,98 @@
+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+int x;
+int y;
+  public:
+int z;
+  protected:
+int a;
+  public:
+int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -238,6 +238,7 @@
readability-container-size-empty
readability-delete-null-pointer
readability-deleted-default
+   readability-duplicated-access-specifiers
readability-else-after-return
readability-function-size
readability-identifier-naming
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -192,6 +192,12 @@
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`readability-magic-numbers
   ` check.
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "ContainerSizeEmptyCheck.h"
 #inclu

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx marked 6 inline comments as done.
m4tx added inline comments.



Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:33
+  for (DeclContext::specific_decl_iterator
+   NS(MatchedDecl->decls_begin()),
+   NSEnd(MatchedDecl->decls_end());

aaron.ballman wrote:
> Why `NS` -- that seems like a strange naming choice.
Sorry, this small piece of code was copied from another place and I forgot to 
change the variable name. I switched to `AS`, hopefully that's more meaningful 
here.


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx marked an inline comment as done.
m4tx added a comment.

In D55793#1333723 , @riccibruno wrote:

> In D55793#1333691 , @m4tx wrote:
>
> > Don't use `CXXRecordDecl::accessSpecs()`, use unique comments in tests.
>
>
> Thanks! `CXXRecordDecl` is already huge and so adding iterators for a single 
> checker is in my opinion not a good idea (especially if the alternative is 
> actually less code).
>  Would it make sense to also issue a diagnostic where the first access 
> specifier is redundant (ie `public` in a `struct`, and `private` in a 
> `class`) ?


Yes, I was thinking about the same thing! However, I believe that some people 
may find this kind of "redundant" access specs actually more readable, so maybe 
it makes sense to add a check option for this to allow users to disable it?


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx added a comment.

In D55793#1333661 , @lebedev.ri wrote:

> Please add tests with preprocessor (`#if ...`) that will show that it ignores 
> disabled code. e.g.:
>
>   class ProbablyValid {
>   private:
> int a;
>   #if defined(ZZ)
>   public:
> int b;
>   #endif
>   private:
> int c;
>   protected:
> int d;
>   public:
> int e;
>   };
>


Is this actually possible? It seems that macros are ran through the 
preprocessor before one can fiddle with them in clang-tidy. In other words, 
`int b` is not at all present in the AST. However, I added a code to detect 
macro expansions, so duplicated access specifiers are ignored if at least one 
of them comes from a macro. If there is a way to cover your case as well, 
please let me know, because even after looking at the code of other checks I 
haven't found out a solution for this.


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx marked 4 inline comments as done.
m4tx added a comment.

In D55793#1335274 , @lebedev.ri wrote:

> In D55793#1335249 , @m4tx wrote:
>
> > In D55793#1333661 , @lebedev.ri 
> > wrote:
> >
> > > Please add tests with preprocessor (`#if ...`) that will show that it 
> > > ignores disabled code. e.g.:
> > >
> > >   class ProbablyValid {
> > >   private:
> > > int a;
> > >   #if defined(ZZ)
> > >   public:
> > > int b;
> > >   #endif
> > >   private:
> > > int c;
> > >   protected:
> > > int d;
> > >   public:
> > > int e;
> > >   };
> > >
> >
> >
> > Is this actually possible?
> >  It seems that macros are ran through the preprocessor before one can 
> > fiddle with them in clang-tidy.
> >  In other words, `int b` is not at all present in the AST.
>
>
> .. and by "ignores" i meant that it **will** be diagnosing this code, since 
> it did not know anything about the code within the preprocessor-disabled 
> section.
>
> > However, I added a code to detect macro expansions, so duplicated access 
> > specifiers are ignored if at least one of them comes from a macro. If there 
> > is a way to cover your case as well, please let me know, because even after 
> > looking at the code of other checks I haven't found out a solution for this.


Ok, thanks for the clarification. I've added the test in the latest diff!




Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:51
+  diag(ASDecl->getLocation(), "duplicated access specifier")
+  << MatchedDecl
+  << FixItHint::CreateRemoval(ASDecl->getSourceRange());

aaron.ballman wrote:
> There is no %0 in the diagnostic string, so I'm not certain what this 
> argument is trying to print out. Did you mean `ASDecl->getSourceRange()` for 
> the underlining?
Sorry, this is another line I forgot to remove. Thanks for pointing this out!

By the way, does the underlining work in clang-tidy's `diag()` function? I see 
it is used outside the project, but here only `FixItHint`s seem to generate 
underline in the generated messages.


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

https://reviews.llvm.org/D55793



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

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx updated this revision to Diff 179138.
m4tx marked an inline comment as done.

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

https://reviews.llvm.org/D55793

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
  clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
  test/clang-tidy/readability-duplicated-access-specifiers.cpp

Index: test/clang-tidy/readability-duplicated-access-specifiers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-duplicated-access-specifiers.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-2{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-3
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-3{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-4
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-4{{$}}
+  int b;
+public:
+  int c;
+};
+
+class FooMacro {
+private:
+  int a;
+#if defined(ZZ)
+  public:
+  int b;
+#endif
+private: // comment-5
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-8]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-5{{$}}
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
+
+class ValidInnerClass {
+public:
+  int a;
+
+  class Inner {
+  public:
+int b;
+  };
+};
+
+#define MIXIN private: int b;
+
+class ValidMacro {
+private:
+  int a;
+MIXIN
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};
Index: docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+int x;
+int y;
+  public:
+int z;
+  protected:
+int a;
+  public:
+int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -238,6 +238,7 @@
readability-container-size-empty
readability-delete-null-pointer
readability-deleted-default
+   readability-duplicated-access-specifiers
readability-else-after-return
readability-function-size
readability-identifier-naming
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -192,6 +192,12 @@
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
 
+- New :doc:`readability-duplicated-access-specifiers
+  ` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`readability-ma