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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D113422#3333992 <https://reviews.llvm.org/D113422#3333992>, @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 polymorphically at the moment so technically `virtual` could be removed, even though it would probably go against developer expectations and might cause confusion for the above reasons in the future. Perhaps doing private instead of public inheritance (//is implemented in terms of//) would communicate the design better in that case?
>
> Do you have profiling data that shows how much overhead the virtual function call actually adds in this particular case? In the projects I've worked with this has typically been a micro-optimization done prematurely without profiling data that supported the need for it, so the benefit was negligible. It did cause a lot of headache to developers reading the code though.

It's definitely negligible as reading globs isn't exactly a hot path, was more just an irk when i was looking over the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422