ilya-biryukov added a comment. In D67172#1662945 <https://reviews.llvm.org/D67172#1662945>, @sammccall wrote:
> Only if it's 5% of something meaningful. If the denominator isn't something > we care about, then it's really "spending XXX usec is not ok" - which depends > on XXX I think. Agree, but unit-test time is meaningful clangd developers. Not saying this is more important than real use-cases, but keeping the unit tests enables us to iterate faster. > One way this could be simpler is if suffix mappings were gone, then the > `StdIncludes` class would go away and we'd just be left with a > `stringmap<string>`. Then there'd be a performance win with almost no extra > complexity. > The description says you're waiting for it to go away, but AFAIK nobody's > working on this nor really knows how to eliminate it. We seem to know how to eliminate this in principle. See @hokein's comment, we could try parsing man pages or even the headers themselves and building a corresponding symbol map. But this patch does not add suffix mapping, so I don't think that's relevant - this patch tries to avoid redundant initialization; not remove suffix maps. > Some of the new complexity seems unneccesary even with this approach. > There's a lot of indirection around the initialization, an enum that gets > passed around a bunch, constructors that switch over it. > I think this could be: Will update the patch. Although I find the current approach more direct, I will happily change the code according to your suggestion. >>> I'm nervous about requiring langopts, it's convenient in >>> tests/prototypes/etc to be able to default-construct CanonicalIncludes >>> (e.g. a test that doesn't actually care about standard library mappings). >> >> We can have a factory method that constructs with default lang opts for >> tests, I don't think this would be an issue in the actual application code. >> In fact, I think it'll make the application code better. > > I'm not saying that there's no way to work around it, or that it's fatal, but > my opinion is that this would be worse than the current state. If we care about having a default constructor, we can default to using `StdIncludes` with a default mapping. I'll avoid requiring `LangOptions` in the constructor in that case. Thanks for the early feedback! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67172/new/ https://reviews.llvm.org/D67172 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
