[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: klimek.
sammccall added a comment.

Sorry to be a pain here, but I'm not sure introducing Javascript is a good idea unless there's a strong reason for it.
All LLVM developers have python installed, many are comfortable with the language - it seems less likely to be a maintenance hazard. I can recommend BeautifulSoup for the HTML parsing stuff.
(For the record, I kind of hate python, but I'm more comfortable reviewing and maintaining it than JS).
Mitigating points: we have some in the VSCode plugin by necessity, and this isn't a build-time dependency.

Happy to get another opinion here.

> Added the tool in this patch, but I think a separate repository under GitHub's clangd org might be a more suitable place.

I'm somewhat sympathetic to this idea, but I don't think we can do it as things stand. @klimek can confirm.



================
Comment at: clangd/StdGen/Readme.md:1
+# StdGen
+
----------------
can we add the caveats here?
 - only symbols directly in namespace std are added
 - symbols with multiple variants or defined in multiple headers aren't added


================
Comment at: clangd/StdGen/Readme.md:3
+
+StdGen is a tool to generate headers for C++ Standard Library symbols by parsing
+archieved HTML files from [cppreference](http://cppreference.com/).
----------------
Can we be a bit more specific about what it generates?

>From this text I'd have guess it generates headers like `namespace std { int printf(char*, ...); }`.


================
Comment at: clangd/StdGen/Readme.md:8
+
+### Requriement
+
----------------
Requriement -> Requirements


================
Comment at: clangd/StdGen/Readme.md:11
+* Download the cppreference [offline HTML files](https://en.cppreference.com/w/Cppreference:Archives),
+and unzip it to the `<StdGen_dir>/reference` directory.
+
----------------
nit: can we make this a command-line arg instead of requiring people to put it in their source tree?


================
Comment at: clangd/StdGen/lib/main.js:1
+const $ = require('cheerio')
+const fs = require('fs')
----------------
note: I haven't reviewed the JS files for style/readability as I'm weak on JS and it's unclear what style would govern here. e.g. I suspect when used as a niche language in a C++ project by people who don't know it well, we should probably have semicolons rather than relying on ASI.
But see code review comment - this may be a moot point.


================
Comment at: clangd/StdGen/lib/parse.js:39
+  })
+  // Skip C++20 symbols as C++20 is not finalized yet.
+  if (revision == 'C++20')
----------------
Not sure this is necessary - it seems preferable to be forwards-compatible even if not 100% accurate, no?

(And it would simplify the code, as we don't need to parse out the version)


================
Comment at: clangd/index/CanonicalIncludes.cpp:110
   static const std::vector<std::pair<const char *, const char *>> SymbolMap = {
       {"std::addressof", "<memory>"},
       // Map symbols in <iosfwd> to their preferred includes.
----------------
can you remove the ones that duplicate the .inc file?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345