[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<AccessSpecDecl>
+           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