[PATCH] D130004: [pseudo] Add `clang-pseudo -html-forest=<output.html>`, an HTML forest browser

Tue Jul 19 06:03:47 PDT 2022

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