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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D67172#1662916 <https://reviews.llvm.org/D67172#1662916>, @ilya-biryukov wrote:

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


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.

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

You'll note that I didn't say that *anything* about the current version was problematic :-)

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.

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:

  StdIncludes *getStdIncludes(LangOpts) {
      if (is c++) {
         static pair<...> entries[] = ...;
         static StdIncludes *CPP = new StdIncludes(entries);
         // the things that apply everywhere are in the StdIncludes constructor
         return CPP;
      } else if (is c) {
         ...
      } else {
        static StdIncludes *Others = new StdIncludes({});
        return Others;
      }
  }



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



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:22
 
-void CanonicalIncludes::addMapping(llvm::StringRef Path,
-                                   llvm::StringRef CanonicalPath) {
-  FullPathMapping[Path] = CanonicalPath;
-}
-
-void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
-                                         llvm::StringRef CanonicalPath) {
-  this->SymbolMapping[QualifiedName] = CanonicalPath;
+enum class StdIncludes::SymbolsFlavour { Cpp, C11, Other };
+namespace {
----------------
Why C11 and not C?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172