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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D55793#1335243 <https://reviews.llvm.org/D55793#1335243>, @m4tx wrote:

> In D55793#1333723 <https://reviews.llvm.org/D55793#1333723>, @riccibruno wrote:
>
> > In D55793#1333691 <https://reviews.llvm.org/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).



================
Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:31
+
+  AccessSpecDecl const *lastAccessDecl = nullptr;
+  for (DeclContext::specific_decl_iterator<AccessSpecDecl>
----------------
aaron.ballman wrote:
> Please switch to `const AccessSpecDecl *`. Also, that should be `LastAccessDecl` per the naming conventions.
Still need to switch where the `const` is.


================
Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:43
+        LastASDecl->getAccess() == ASDecl->getAccess()) {
+      // Ignore macro expansions
+      if (ASDecl->getLocation().isMacroID() ||
----------------
Add a full stop at the end of the sentence.


================
Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:51
+      diag(ASDecl->getLocation(), "duplicated access specifier")
+          << MatchedDecl
+          << FixItHint::CreateRemoval(ASDecl->getSourceRange());
----------------
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?


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

https://reviews.llvm.org/D55793