hokein added inline comments.
================
Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py - -------------------------------------------*- python
-*--===#
+#
----------------
ioeric wrote:
> 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.
Changed to include-mapping/gen_std.py, according to offline discussion.
================
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)
----------------
ioeric wrote:
> 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.
> 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 any big difference between checking `exists(index_page_path)` and
`exists(cpp_symbol_root)` here. Checking `cpp_symbol_root` is more natural as
we also construct the path of symbol page from it.
Anyway, changed to check index_page_path.
================
Comment at: clangd/StdSymbolMap.inc:8
+//===----------------------------------------------------------------------===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
----------------
ioeric wrote:
> 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.
> If we can't get this from the doc, we should ask users to explicitly provide
> this information.
I'm not sure this is a good idea. Asking users provide the information seems
error-prone.
I think we may use the file stat information (modified date) of the
`symbol_index.html` page.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58345/new/
https://reviews.llvm.org/D58345
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
