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