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

[m4tx]

Summary

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

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

[Eugene.Zelenko]

Please close PR25403 after commit.

docs/ReleaseNotes.rst

70 ↗

(On Diff #178536)

Please use alphabetical order for new checks.

[m4tx]

Use alphabetical order in ReleaseNotes.rst

[lebedev.ri]

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;
};

[m4tx]

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

[riccibruno]

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

[Eugene.Zelenko]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

36 ↗

(On Diff #178543)

Type is not obvious here, so please don't use auto.

[aaron.ballman]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

21 ↗

(On Diff #178543)

You should only register this matcher in C++ mode.

31 ↗

(On Diff #178543)

Please switch to const AccessSpecDecl *. Also, that should be LastAccessDecl per the naming conventions.

33 ↗

(On Diff #178543)

Why NS -- that seems like a strange naming choice.

36 ↗

(On Diff #178543)

Also, decl doesn't match our naming conventions -- change to ASDecl?

[m4tx]

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

[m4tx]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

33 ↗

(On Diff #178543)

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.

[m4tx]

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?

[m4tx]

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.

[lebedev.ri]

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.

[riccibruno]

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?

Perhaps ? I will leave someone with more experience here to comment.

[aaron.ballman]

In D55793#1335243, @m4tx wrote:

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?

I'm on the fence about whether this needs to be an option or not. Perhaps having some data on the rate of diagnosis would be helpful here -- can you try adding the feature and running it over some large code bases to see if the implicit access spec warning triggers a lot more than expected? Also, I kind of think this check should be named readability-redundant-access-specifiers instead of duplicated if it's going to consider implicit access specifiers (since the user doesn't write those, "duplicate" may be surprising).

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

31 ↗

(On Diff #178543)

Still need to switch where the const is.

43 ↗

(On Diff #178777)

Add a full stop at the end of the sentence.

51 ↗

(On Diff #178777)

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?

[m4tx]

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!

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

51 ↗

(On Diff #178777)

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 FixItHints seem to generate underline in the generated messages.

[aaron.ballman]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

51 ↗

(On Diff #178777)

That's a good question that I don't actually know the answer to. :-D I believe it still works -- you can try it out by passing a SourceRange to the diag() call, like:

diag(LastASDecl->getLocation(), "duplicated access specifier") << SomeSourceRange;

You should see the range from SomeSourceRange underlined.

[JonasToth]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

51 ↗

(On Diff #178777)

I believe, if you pass in a token or tokenrange or so, the underlying exists. I thought i got suprised by something like that once, not sure though.

[m4tx]

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.

[m4tx]

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

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

35

Please use double back-ticks for language keywords, like private anc class.

48

Please use single back-ticks for option names.

[m4tx]

Updated the backticks in the documentation.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp

36–37

I have a slight preference for using assignment operators here rather than explicit constructor calls.

55

This is a bit terse, how about: redundant access specifier has the same accessibility as the implicit access specifier?

70

This is a bit terse, how about: redundant access specifier has the same accessibility as the previous access specifier?

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

7

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.

34

give warning -> diagnose

35

May also want to put public inside struct or union as well.

49

since -> because

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

72–73

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.

[m4tx]

Updated per @aaron.ballman comments

[m4tx]

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp

36–37

This is not possible here as specific_decl_iterator has the copy constructor marked as explicit.

55

Sounds good, changed this in the latest revision.

70

As above.

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

7

This is actually included in the documentation (*member* access specifier), but I've added explicit "field and method" clarification.

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

72–73

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.

[m4tx]

Remove the accidentally added patch file

[aaron.ballman]

LGTM!

[JonasToth]

Given this revision is accepted and seems ready to go: Do you need someone to commit on your behalf?

[m4tx]

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

[aaron.ballman]

In D55793#1535895, @m4tx wrote:

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

I'm sorry for the terribly long delay on getting this committed -- it fell off my radar for a bit. When I try to apply the branch to trunk, I get merge conflicts. Can you rebase it on ToT, please?

[twardakm]

Rebase with master

[twardakm]

In D55793#1571534, @aaron.ballman wrote:

In D55793#1535895, @m4tx wrote:

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

I'm sorry for the terribly long delay on getting this committed -- it fell off my radar for a bit. When I try to apply the branch to trunk, I get merge conflicts. Can you rebase it on ToT, please?

I rebased the commit with current master, run the tests and everything looks fine. @JonasToth can you take a look and push the changes?

[aaron.ballman]

I've commit on your behalf in 29dc0b17de6b04afa6110a040053a19b02ca1a87, thank you for the patch!

[Eugene.Zelenko]

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

[aaron.ballman]

In D55793#1727423, @Eugene.Zelenko wrote:

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

Both should be done now, thanks!

[Eugene.Zelenko]

In D55793#1727437, @aaron.ballman wrote:

In D55793#1727423, @Eugene.Zelenko wrote:

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

Both should be done now, thanks!

Thank you for quick fix!

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
+  RedundantAccessSpecifiersCheck.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
+#include "RedundantAccessSpecifiersCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"

         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<RedundantAccessSpecifiersCheck>(
+        "readability-redundant-access-specifiers");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.h

+//===--- RedundantAccessSpecifiersCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTACCESSSPECIFIERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTACCESSSPECIFIERSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detects redundant access specifiers inside classes, structs, and unions.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-access-specifiers.html
+class RedundantAccessSpecifiersCheck : public ClangTidyCheck {
+public:
+  RedundantAccessSpecifiersCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        CheckFirstDeclaration(
+            Options.getLocalOrGlobal("CheckFirstDeclaration", false)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool CheckFirstDeclaration;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTACCESSSPECIFIERSCHECK_H

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp

+//===--- RedundantAccessSpecifiersCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantAccessSpecifiersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantAccessSpecifiersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      cxxRecordDecl(has(accessSpecDecl())).bind("redundant-access-specifiers"),
+      this);
+}
+
+void RedundantAccessSpecifiersCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("redundant-access-specifiers");
+
+  const AccessSpecDecl *LastASDecl = nullptr;
+  for (DeclContext::specific_decl_iterator<AccessSpecDecl>
+           AS(MatchedDecl->decls_begin()),
+       ASEnd(MatchedDecl->decls_end());
+       AS != ASEnd; ++AS) {
+    const AccessSpecDecl *ASDecl = *AS;
+
+    // Ignore macro expansions.
+    if (ASDecl->getLocation().isMacroID()) {
+      LastASDecl = ASDecl;
+      continue;
+    }
+
+    if (LastASDecl == nullptr) {
+      // First declaration.
+      LastASDecl = ASDecl;
+
+      if (CheckFirstDeclaration) {
+        AccessSpecifier DefaultSpecifier =
+            MatchedDecl->isClass() ? AS_private : AS_public;
+        if (ASDecl->getAccess() == DefaultSpecifier) {
+          diag(ASDecl->getLocation(),
+               "redundant access specifier has the same accessibility as the "
+               "implicit access specifier")
+              << FixItHint::CreateRemoval(ASDecl->getSourceRange());
+        }
+      }
+
+      continue;
+    }
+
+    if (LastASDecl->getAccess() == ASDecl->getAccess()) {
+      // Ignore macro expansions.
+      if (LastASDecl->getLocation().isMacroID()) {
+        LastASDecl = ASDecl;
+        continue;
+      }
+
+      diag(ASDecl->getLocation(),
+           "redundant access specifier has the same accessibility as the "
+           "previous access specifier")
+          << FixItHint::CreateRemoval(ASDecl->getSourceRange());
+      diag(LastASDecl->getLocation(), "previously declared here",
+           DiagnosticIDs::Note);
+    } else {
+      LastASDecl = ASDecl;
+    }
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

   but either don't specify it or the clause is specified but with the kind
   other than ``none``, and suggests to use the ``default(none)`` clause.
 
+- New :doc:`readability-redundant-access-specifiers
+  <clang-tidy/checks/readability-redundant-access-specifiers>` check.
+
+  Finds classes, structs, and unions that contain redundant member
+  access specifiers.
+
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
+   readability-redundant-access-specifiers
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

+.. title:: clang-tidy - readability-redundant-access-specifiers
+
+readability-redundant-access-specifiers
+=======================================
+
+Finds classes, structs, and unions containing redundant member (field and
+method) access specifiers.
+
+Example
+-------
+
+.. 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.
+
+Options
+-------
+
+.. option:: CheckFirstDeclaration
+
+   If set to non-zero, the check will also diagnose if the first access
+   specifier declaration is redundant (e.g. ``private`` inside ``class``,
+   or ``public`` inside ``struct`` or ``union``).
+   Default is `0`.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+  struct Bar {
+  public:
+    int x;
+  }
+
+If `CheckFirstDeclaration` option is enabled, a warning about redundant
+access specifier will be emitted, because ``public`` is the default member access
+for structs.

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers-check-first-declaration.cpp

+// 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-specifiers]
+  // CHECK-FIXES: {{^}}// comment-1{{$}}
+  int a;
+};
+
+union UnionPublic {
+public: // comment-2
+  // 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-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
+};

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

+// 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;
+};