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