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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 8 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:41
 
+  if (!SuffixHeaderMapping)
+    return Header;
----------------
sammccall wrote:
> nit: can we write `if (SuffixHeaderMapping) { ... }` for consistency with above?
I'd prefer less nesting to consistency, but happy let me know if you feel that's very important.


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:84
+  llvm::StringMap<std::string> SuffixHeaderMapping;
+  int MaxSuffixComponents = 0;
+};
----------------
sammccall wrote:
> 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?
The assertion is inside the loop that's adding the mappings instead of `llvm::all_of`. 


================
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},
----------------
sammccall wrote:
> if you want to save CPU, move to the scope where it's used? Lit tests and many interesting subsets will never use C.
Done. Also initialized everything directly as StringMap


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67172