carlosgalvezp added a comment. In D113422#3333748 <https://reviews.llvm.org/D113422#3333748>, @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` //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. 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
