[PATCH] D63270: [clangd] Add include-mapping for C symbols.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/include-mapping/cppreference_parser.py:1
-#!/usr/bin/env python
-#===- gen_std.py -  ------------------------------------------*- python -*--===#
----------------
could we add a similar License and header comment section to this file as well ?


================
Comment at: clang-tools-extra/clangd/include-mapping/gen_std.py:90
+    symbol_index_root = page_root
+    parse_pages = [(page_root, "index.html", "")]
+
----------------
maybe we should rather pass some something like "INVALID" as namespace for C symbols?


================
Comment at: clang-tools-extra/clangd/include-mapping/gen_std.py:95
 
-  parse_pages =  [
-    (cpp_root, "symbol_index.html", "std::"),
-    # std sub-namespace symbols have separated pages.
-    # We don't index std literal operators (e.g.
-    # std::literals::chrono_literals::operator""d), these symbols can't be
-    # accessed by std::<symbol_name>.
-    # FIXME: index std::placeholders symbols, placeholders.html page is
-    # different (which contains one entry for _1, _2, ..., _N), we need special
-    # handling.
-    (symbol_index_root, "chrono.html", "std::chrono::"),
-    (symbol_index_root, "filesystem.html", "std::filesystem::"),
-    (symbol_index_root, "pmr.html", "std::pmr::"),
-    (symbol_index_root, "regex_constants.html", "std::regex_constants::"),
-    (symbol_index_root, "this_thread.html", "std::this_thread::"),
-  ]
-
-  symbols = []
-  # Run many workers to process individual symbol pages under the symbol index.
-  # Don't allow workers to capture Ctrl-C.
-  pool = multiprocessing.Pool(
-      initializer=lambda: signal.signal(signal.SIGINT, signal.SIG_IGN))
-  try:
-    for root_dir, page_name, namespace in parse_pages:
-      symbols.extend(GetSymbols(pool, root_dir, page_name, namespace))
-  finally:
-    pool.terminate()
-    pool.join()
+  symbols = cppreference_parser.GetSymbols(parse_pages)
 
----------------
I believe it is more sensible for this function to take `(args.cppreference, args.language)` and perform the above mentioned logic to generate `parse_pages` from these two internally.


================
Comment at: clang-tools-extra/clangd/include-mapping/test.py:1
 #!/usr/bin/env python
 #===- test.py -  ---------------------------------------------*- python -*--===#
----------------
no new tests for c symbol parsing ?


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:97
+  static const std::vector<std::pair<const char *, const char *>> CSymbolMap = {
+#define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name, #Header},
+      #include "CSymbolMap.inc"
----------------
Let's drop the `#NameSpace` part from the expansion


================
Comment at: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp:19
+  auto Language = LangOptions();
+  Language.CPlusPlus = true;
+  addSystemHeadersMapping(&CI, Language);
----------------
could you also add some tests for C?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63270