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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
