[PATCH] D67172: [clangd] Use pre-populated mappings for standard symbols

Mon Sep 9 04:46:36 PDT 2019

ilya-biryukov added a comment.

In D67172#1662888 <https://reviews.llvm.org/D67172#1662888>, @sammccall wrote:

> I do wonder whether we're microoptimizing for the tests too much, I don't think 5% on the tests is worth very much in itself, unless it's speeding up real workloads or improving the code (it may well be).

Even though tests don't parse real C++ programs, I wouldn't call it a micro-optimization. Spending 5% on such a low-level operation is not ok, small things like that can add up and have a subtle effect on performance.
If there's an issue with making the code more complex, I'm happy to explore other ways to make it simpler. What would convince you that's a cleanup that improves things? What parts of the current version do you think are problematic?

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172