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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, though if we can drop the struct and make MaxSuffixComponents a constant it'd be simpler still.

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

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


Sure. This is going to win a couple of percent I guess: for these cases we care about walltime including rebuild and lit startup overhead. It's worth having (and thanks for doing this!) but I don't think we should pay much complexity.

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

"we could try" sounds like we *don't* know how to eliminate it. Parsing manpages aside, I thought the main problem was these symbols are nonstandard and an infinitely portable qname->header mapping simply didn't exist.

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

This patch adds code shoveling around suffix maps, and so increases the cost of this feature. (Less so after latest changes). It also increases the scope of a future patch that would remove suffix maps. Sequencing this the other way would be better, if removing suffix mapping was straightforward. (I don't think it is)

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

Thanks, this looks a lot better!
I think it can be simplified a little further, as the suffix maps are totally hardcoded now.



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:41
 
+  if (!SuffixHeaderMapping)
+    return Header;
----------------
nit: can we write `if (SuffixHeaderMapping) { ... }` for consistency with above?


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:84
+  llvm::StringMap<std::string> SuffixHeaderMapping;
+  int MaxSuffixComponents = 0;
+};
----------------
so this is a constant, and it's 3.

Can we avoid the calculation/propagation and defining a struct, and just add an assert(llvm::all_of(...)) after the initialization?


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:91
   //   - symbols with multiple headers (e.g. std::move)
   static constexpr std::pair<const char *, const char *> SystemHeaderMap[] = {
       {"include/__stddef_max_align_t.h", "<cstddef>"},
----------------
I think this could just be
```
static const auto *SystemHeaderMap = new llvm::StringMap<string>{
...
}
```
skipping the intermediate array, and doing the initialize-once here
(if we can drop the struct)


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:758
+void CanonicalIncludes::addSystemHeadersMapping(const LangOptions &Language) {
+  static constexpr std::pair<const char *, const char *> SymbolMap[] = {
+#define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name, #Header},
----------------
if you want to save CPU, move to the scope where it's used? Lit tests and many interesting subsets will never use C.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:60
   /// A map from a suffix (one or components of a path) to a canonical path.
-  llvm::StringMap<std::string> SuffixHeaderMapping;
+  /// Used only for mapping standard headers.
+  const llvm::StringMap<std::string> *SuffixHeaderMapping = nullptr;
----------------
nit: maybe StdSuffixHeaderMapping to clarify the purpose, now these are std-only


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172