[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood abandoned this revision. LegalizeAdulthood added a comment. Abandoning this in favor of https://reviews.llvm.org/D117409 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117129/new/ https://reviews.llvm.org/D117129 ___ cfe-comm

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

aaron.ballman added a comment. In D117129#3251705 , @njames93 wrote: > In D117129#3251577 , @aaron.ballman > wrote: > >> In D117129#3246403 , @njames93 >> wrote: >> >>>

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

njames93 added a comment. In D117129#3251577 , @aaron.ballman wrote: > In D117129#3246403 , @njames93 > wrote: > >> There is a hurdle to overcome with this, Currently each check can have its >> own include styl

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

aaron.ballman added a comment. In D117129#3246403 , @njames93 wrote: > In D117129#3239514 , > @LegalizeAdulthood wrote: > >> In D117129#3239143 , @njames93 >> wrote: >>

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood added a comment. I added some comments on the other review. Is constructing an `IncludeInserter` particularly expensive? I'm just curious. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117129/new/ https://reviews.llvm.org/D117129 _

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

njames93 added a comment. In D117129#3246610 , @LegalizeAdulthood wrote: > In D117129#3246403 , @njames93 > wrote: > >> > > I pushed the functionality down into ClangTidyCheck > instead of introducing an interm

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood added a comment. In D117129#3246403 , @njames93 wrote: > There is a hurdle to overcome with this, Currently each check can have its > own include style - which doesn't really make sense. Hopefully people > actually just use the global

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood updated this revision to Diff 400352. LegalizeAdulthood added a comment. Eliminate an intermediate class and move the IncludeInserter functionality into ClangTidyCheck with an enum to indicate opt-in to use of the IncludeInserter. CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

njames93 added a comment. In D117129#3239514 , @LegalizeAdulthood wrote: > In D117129#3239143 , @njames93 > wrote: > >> My proposed idea was putting the `IncludeInserter` instance >> inside the `ClangTidyContext

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood added a comment. In D117129#3239143 , @njames93 wrote: > My proposed idea was putting the `IncludeInserter` instance > inside the `ClangTidyContext`, but then only registering it if > any checks actually want to use it. Use of the `Incl

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

njames93 added a comment. In D117129#3238441 , @LegalizeAdulthood wrote: > Another option would be to migrate this to `ClangTidyCheck` itself > and add a flag/option passed to the ClangTidyCheck c'tor to opt-in > to the feature? I feel that this directi

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

ymandel added a comment. In D117129#3238441 , @LegalizeAdulthood wrote: > Another option would be to migrate this to `ClangTidyCheck` itself > and add a flag/option passed to the ClangTidyCheck c'tor to opt-in > to the feature? Right, I was wondering ab

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood added a comment. Another option would be to migrate this to `ClangTidyCheck` itself and add a flag/option passed to the ClangTidyCheck c'tor to opt-in to the feature? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117129/new/ https

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood added a comment. In D117129#3238433 , @ymandel wrote: > Overall, this change looks good and makes sense. But, it doesn't scale past 1 > feature > [...] Might there be some other way to get the benefits of saving > implementers the > cu

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

ymandel added a comment. Overall, this change looks good and makes sense. But, it doesn't scale past 1 feature, which makes me a bit hesitant. I don't have another example offhand, but include-inserter is an arbitrary feature and I don't love conflating feature addition with the class hierarchy

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

LegalizeAdulthood created this revision. LegalizeAdulthood added a reviewer: alexfh. LegalizeAdulthood added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, arphaman, kbarton, xazax.hun, mgorny, nemanjai. LegalizeAdulthood requested review of this revision. IncluderClangTid