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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
