[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

njames93 added a comment. In D113422#992 , @carlosgalvezp wrote: > Nothing other than readability, `CachedGlobList` //is a// `GlobList` so it > made sense to implement it with standard, by-the-book inheritance. But no, > it's not used

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added a comment. In D113422#748 , @njames93 wrote: > Was there any real use case for making this polymorphic, The overhead for a > virtual function call seems a little unnecessary IMO? Nothing other than readability, `CachedGlobList`

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

njames93 added a comment. Was there any real use case for making this polymorphic, The overhead for a virtual function call seems a little unnecessary IMO? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added a comment. Thanks a lot for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422 ___ cfe-commits mailing list

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

This revision was automatically updated to reflect the committed changes. Closed by commit rG946eb7a037d5: [clang-tidy][NFC] Move CachedGlobList to GlobList.h (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM. The use of `mutable` with public inheritence is all over the clang-tools-extra code. See `class SwapIndex : public SymbolIndex`. CHANGES SINCE LAST ACTION

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

salman-javed-nz added a comment. Will aim to review and come back to you in a couple of days. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp updated this revision to Diff 390708. carlosgalvezp added a comment. Switch to public inheritance + mutable cached data. This seems to be a valid use case for mutable, even though I'm not sure if there's anything against it in the LLVM

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added a comment. Kind ping to reviewers. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp updated this revision to Diff 387304. carlosgalvezp added a comment. - Replace composition with private inheritance. - Fix namespaces. Public inheritance is not possible due to the base class' function being const-qualified; the derived class cannot be const since it updates state

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52 +/// A \p GlobList that caches search results, so that search is performed only +/// once for the same query. +class CachedGlobList { carlosgalvezp wrote: > whisperity

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52 +/// A \p GlobList that caches search results, so that search is performed only +/// once for the same query. +class CachedGlobList { whisperity wrote: > If

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52 +/// A \p GlobList that caches search results, so that search is performed only +/// once for the same query. +class CachedGlobList { If `CachedGlobList` //is-a//

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp added a comment. Kind ping @aaron.ballman @whisperity Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422 ___ cfe-commits mailing list

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

carlosgalvezp created this revision. carlosgalvezp added reviewers: aaron.ballman, whisperity. Herald added subscribers: rnkovacs, xazax.hun. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Currently it's hidden