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