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