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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:35-36
+  for (DeclContext::specific_decl_iterator<AccessSpecDecl>
+           AS(MatchedDecl->decls_begin()),
+       ASEnd(MatchedDecl->decls_end());
+       AS != ASEnd; ++AS) {
----------------
I have a slight preference for using assignment operators here rather than explicit constructor calls.


================
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());
----------------
This is a bit terse, how about: `redundant access specifier has the same accessibility as the implicit access specifier`?


================
Comment at: clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:69
+
+      diag(ASDecl->getLocation(), "duplicated access specifier")
+          << FixItHint::CreateRemoval(ASDecl->getSourceRange());
----------------
This is a bit terse, how about: `redundant access specifier has the same accessibility as the previous access specifier`?


================
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.
+
----------------
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.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst:33
+
+   If set to non-zero, the check will also give warning if the first access
+   specifier declaration is redundant (e.g. ``private`` inside ``class``).
----------------
give warning -> diagnose


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst:34
+   If set to non-zero, the check will also give warning if the first access
+   specifier declaration is redundant (e.g. ``private`` inside ``class``).
+   Default is `0`.
----------------
May also want to put `public` inside `struct` or `union` as well.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst:48
+If `CheckFirstDeclaration` option is enabled, a warning about redundant
+access specifier will be emitted, since ``public`` is the default member access
+for structs.
----------------
since -> because


================
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{{$}}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55793