hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land.
Super cool! Can't wait to use it. The code looks good, just nits (feel free to ignore) ================ Comment at: clang-tools-extra/pseudo/tool/CMakeLists.txt:29 +add_custom_target(clang-pseudo-resources DEPENDS HTMLForestResources.inc) +add_dependencies(clang-pseudo clang-pseudo-resources) ---------------- This likely introduces some pain. (An alternative is to inline the content of `.js`, `.css`, `html` in the cpp directly, but it is awkward). Would be nice to prepare an internal BUILD file change when landing (if you don't have time, I'm happy to help with it). ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.cpp:64 + void write() { + Out << "<!doctype html>\n<html>\n<head>\n<title>HTMLForest</title>\n"; + Out << "<script>" << HTMLForest_js << "</script>\n"; ---------------- nit: it is clearer to use use `llvm::formatv` ``` llvm::formatv(R"html( ... )html", js, css, forest_json, code, html);. ``` ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.cpp:115 + llvm::DenseMap<const ForestNode *, unsigned> Index; + std::vector<std::pair<const ForestNode *, /*End*/ Token::Index>> Queue; + auto AssignID = [&](const ForestNode* N, Token::Index End) -> unsigned { ---------------- I'd suggest renaming to `Array` and adding comment for the `Index` (it is the index in the `Queue`), `Queue` leaves an impression to me that we will do some pop_back. ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.cpp:170 + +// We only accept the derived stream here. Would it be useful or confusing +// to allow the original stream instead? ---------------- Having derived stream is fine to me at the moment, particularly useful for debugging. But as a regular user, I would prefer to see the original token stream (the derived stream is confusing, as a regular user doesn't know how pseudoparser work inside). ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.css:50 +} +#i_kind { + float: right; ---------------- nit: just an idea we could use different background color for different forest node kind. ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.css:63 + +#tree { + flex-grow: 0; ---------------- nit: can we make the tree and code view resizable? so that I can adjust the width of tree/code view. ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.css:82 +.tree-node.terminal .name { font-family: monospace; } +.tree-node.ambiguous > header .name { color: #803; font-weight: bold; } + ---------------- nit: we could also add some style (possibly red indicating the error) for opaque node. ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.html:1 +<div id="tree"><ul></ul></div> +<pre id="code"></pre> ---------------- I think it might be better to move these `.html, .css, .js` files to a separate directory `tool/resources` rather than `tool`. ================ Comment at: clang-tools-extra/pseudo/tool/HTMLForest.html:13 + <section> + <div id="i_context"></div> + </section> ---------------- nit: maybe name it `i_context_stack`, I had no idea what does the `context` mean until I read the actual implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130004/new/ https://reviews.llvm.org/D130004 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits