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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  -------------------------------------------*- python -*--===#
+#
----------------
hokein wrote:
> ioeric wrote:
> > I'd avoid abbreviation in the file name and the new directory name. `StdGen` sounds a lot like a library ;)
> > 
> > I'd suggest something like `std-include-mapping/generate.py`
> Personally I'd vote `StdGen`, and it follows llvm's `TableGen`.  The name `std-include-mapping/generate.py` seems too verbose...
Why do we want to follow `TableGen`?

Two reasons why I think a more informative name should be used. 1) `StdGen` sounds like a name for library and doesn't follow the naming convention for tool directory (e.g. `global-symbol-builder`) and 2) it's hard for readers without much context to understand what `StdGen` actually means.


================
Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+    exit("Path %s doesn't exist!" % cpp_reference_root)
----------------
hokein wrote:
> ioeric wrote:
> > why not check `exists(index_page_path)` instead?
> I think either way works.
It seems to me that using `exists(index_page_path)` would be less likely to trigger an exception (better use experience). I don't see a good reason to check the root.


================
Comment at: clangd/StdSymbolMap.inc:8
+//===----------------------------------------------------------------------===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
----------------
hokein wrote:
> ioeric wrote:
> > can we include the information about the HTML archive (e.g. STL version) from which this table is generated? This would be useful for future maintenance.
> The version of HTML archive is the date information, but this information is only present in the `.zip` name, we lose it after we unzip the `zip` file....
If we can't get this from the doc, we should ask users to explicitly provide this information. I think we would want some version info to relate the output to the source.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345