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

Tue Jul 19 11:03:38 PDT 2022

sammccall marked 7 inline comments as done.
sammccall added a comment.

Thanks for the review!

================
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)
----------------
hokein wrote:
> 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).
> This likely introduces some pain. (An alternative is to inline the content of `.js`, `.css`, `html` in the cpp directly, but it is awkward).
Yeah, I think this is unreasonably difficult to edit, unfortunately - no editor understands JS/CSS/html inside string literals. (We can probably get away with the HTML, but the JS is a significant amount of code)

(In fact had a second mode where the resources were read from disk without a recompile, which was faster while working on it, but I don't think it's necessary - inotifywait loop is enough)

> 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).
Good point, I'll prepare that.

================
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";
----------------
hokein wrote:
> nit: it is clearer to use use `llvm::formatv`
> 
> ```
> llvm::formatv(R"html(
> ...
> )html", js, css, forest_json, code, html);.
> ```
I guess, but:
 - a long formatv string means it's hard to spot which {n} corresponds to which arg
 - we still have to break up the formatv for writeForestJSON() etc, which doesn't seem like a natural place to break.

Added a small tag(name, block) function instead which I think makes the structure easier to follow (basically the clang-formatted code shows the HTML structure)

================
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 {
----------------
hokein wrote:
> 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.
I agree about Queue.
`Array` is confusing as this isn't actually either a C++ array or a `json::Array`.
Chose `Sequence` to emphasize that it's the order we're establishing.

================
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?
----------------
hokein wrote:
> 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).
Yeah, I think we'll want this later changed to FIXME.
We will have to assume some invariants to get it (i.e. that the derived tokens are basically a same-order subset of the original ones).
Not sure if "regular users" will ever see this tool :-) I'm not sure it extends well to forests, we may want something else.

================
Comment at: clang-tools-extra/pseudo/tool/HTMLForest.css:50
+}
+#i_kind {
+  float: right;
----------------
hokein wrote:
> nit: just an idea we could use different background color for different forest node kind.
Done, and made the colors of the nodes in the tree match.
I like it, hope it's not too rainbow-y

================
Comment at: clang-tools-extra/pseudo/tool/HTMLForest.css:63
+
+#tree {
+  flex-grow: 0;
----------------
hokein wrote:
> nit: can we make the tree and code view resizable?  so that I can adjust the width of tree/code view.
`resize: horizontal` seems to do the trick

================
Comment at: clang-tools-extra/pseudo/tool/HTMLForest.html:1
+<div id="tree"><ul></ul></div>
+<pre id="code"></pre>
----------------
hokein wrote:
> I think it might be better to move these `.html, .css, .js` files to a separate directory  `tool/resources` rather than `tool`.
the tool/ directory is ~empty or I would... happy to add some more structure if it gets at all crowded

================
Comment at: clang-tools-extra/pseudo/tool/HTMLForest.html:13
+  <section>
+    <div id="i_context"></div>
+  </section>
----------------
hokein wrote:
> nit: maybe name it `i_context_stack`, I had no idea what does the `context` mean until I read the actual implementation. 
Renamed to `i_ancestors` which seems explicit without being long

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130004