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

Eugene.Zelenko added a comment. In D55793#1727437 , @aaron.ballman wrote: > In D55793#1727423 , @Eugene.Zelenko > wrote: > > > @aaron.ballman: Please move Release Notes entry to new checks section (in > > alphabe

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

aaron.ballman added a comment. In D55793#1727423 , @Eugene.Zelenko wrote: > @aaron.ballman: Please move Release Notes entry to new checks section (in > alphabetical order). Currently it's located in //Improvements to > include-fixer//. Please also close

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

Eugene.Zelenko added a comment. @aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in //Improvements to include-fixer//. Please also close PR. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

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

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit on your behalf in 29dc0b17de6b04afa6110a040053a19b02ca1a87 , thank you for the patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

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

twardakm added a comment. In D55793#1571534 , @aaron.ballman wrote: > In D55793#1535895 , @m4tx wrote: > > > @JonasToth thanks for asking! Yes, since I do not have commit access, I > > need someone to do the commi

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

twardakm updated this revision to Diff 225939. twardakm added a comment. Rebase with master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt

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

aaron.ballman added a comment. In D55793#1535895 , @m4tx wrote: > @JonasToth thanks for asking! Yes, since I do not have commit access, I need > someone to do the commit for me. I'm sorry for the terribly long delay on getting this committed -- it fell

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

m4tx added a comment. @JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 ___ cfe-commits ma

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

JonasToth added a comment. Given this revision is accepted and seems ready to go: Do you need someone to commit on your behalf? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 ___ cfe-commits mailin

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 ___ cfe-commits mailing lis

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

m4tx updated this revision to Diff 197028. m4tx marked 5 inline comments as done. m4tx added a comment. Remove the accidentally added patch file CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tools-extra/clang-tidy/readability/CM

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

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 + AS(MatchedDecl->decls_begin()), + ASEnd(MatchedDecl->de

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

m4tx updated this revision to Diff 197025. m4tx added a comment. Updated per @aaron.ballman comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: 0001-clang-tidy-Add-duplicated-access-specifier-readabili.patch clang-tools-extra/cla

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp:35-36 + for (DeclContext::specific_decl_iterator + AS(MatchedDecl->decls_begin()), + ASEnd(MatchedDecl->decls_end()); + AS != ASEnd

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

m4tx updated this revision to Diff 195409. m4tx added a comment. Updated the backticks in the documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tools-extra/clang-tidy/readability

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

Eugene.Zelenko added inline comments. 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 `c

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

m4tx updated this revision to Diff 195408. m4tx added a comment. Herald added a project: clang. Updated the diff for the latest (master) version, used GitHub monorepo for the changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https:/

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

m4tx added a comment. I've added the implicit access specifier check and run it on some bigger codebases. My findings are as below: - Dolphin: 6 //triggers across// 856 //record types// - OpenCV: 31 //triggers across// 3370 //record types// - Inkscape: 39 //triggers across// 846 //record types//

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

m4tx updated this revision to Diff 181474. m4tx marked an inline comment as not done. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/Rea

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

JonasToth added inline comments. Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:51 + diag(ASDecl->getLocation(), "duplicated access specifier") + << MatchedDecl + << FixItHint::CreateRemoval(ASDecl->getSourceRange()); ---

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

aaron.ballman added inline comments. Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:51 + diag(ASDecl->getLocation(), "duplicated access specifier") + << MatchedDecl + << FixItHint::CreateRemoval(ASDecl->getSourceRange()); ---

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

m4tx updated this revision to Diff 179138. m4tx marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp clang-t

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

m4tx marked 4 inline comments as done. m4tx added a comment. In D55793#1335274 , @lebedev.ri wrote: > In D55793#1335249 , @m4tx wrote: > > > In D55793#1333661 , @lebedev.ri

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

aaron.ballman added a comment. In D55793#1335243 , @m4tx wrote: > In D55793#1333723 , @riccibruno > wrote: > > > In D55793#1333691 , @m4tx wrote: > > > > > Don't use `CXXRec

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

riccibruno added a comment. > 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? Perhaps ? I will leave

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

lebedev.ri added a comment. In D55793#1335249 , @m4tx wrote: > In D55793#1333661 , @lebedev.ri > wrote: > > > Please add tests with preprocessor (`#if ...`) that will show that it > > ignores disabled code. e.g.:

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

m4tx added a comment. In D55793#1333661 , @lebedev.ri wrote: > Please add tests with preprocessor (`#if ...`) that will show that it ignores > disabled code. e.g.: > > class ProbablyValid { > private: > int a; > #if defined(ZZ) > public: >

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

m4tx marked an inline comment as done. m4tx added a comment. In D55793#1333723 , @riccibruno wrote: > In D55793#1333691 , @m4tx wrote: > > > Don't use `CXXRecordDecl::accessSpecs()`, use unique comments in tests. >

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

m4tx marked 6 inline comments as done. m4tx added inline comments. Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:33 + for (DeclContext::specific_decl_iterator + NS(MatchedDecl->decls_begin()), + NSEnd(MatchedDecl->decls_end()); --

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

m4tx updated this revision to Diff 178777. m4tx added a comment. Fix variable names, add macro expansion checking as well as macro and inner class tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tidy/readability/CMakeLists

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

aaron.ballman added inline comments. Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:21 +void DuplicatedAccessSpecifiersCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxRecordDecl(has(accessSpecDecl())) You sh

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

Eugene.Zelenko added inline comments. Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:36 + NS != NSEnd; ++NS) { +const auto *decl = *NS; + Type is not obvious here, so please don't use auto. CHANGES SINCE LAST ACTION https://r

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

riccibruno added a comment. In 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

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

m4tx updated this revision to Diff 178543. m4tx added a comment. Don't use `CXXRecordDecl::accessSpecs()`, use unique comments in tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/

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

lebedev.ri added a comment. Please add tests with preprocessor (`#if ...`) that will show that it ignores disabled code. e.g.: class ProbablyValid { private: int a; #if defined(ZZ) public: int b; #endif private: int c; protected: int d; public: int e; }; C

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

m4tx updated this revision to Diff 178539. m4tx added a comment. Use alphabetical order in ReleaseNotes.rst CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55793/new/ https://reviews.llvm.org/D55793 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DuplicatedAccessS

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

Eugene.Zelenko added a comment. Please close PR25403 after commit. Comment at: docs/ReleaseNotes.rst:70 +- New :doc:`readability-duplicated-access-specifiers + ` check. Please use alphabetical order for new checks. Repository: rCTE Clang Tools Extra CHA