[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

doh, forgot to send



================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:128
+    *OS << "<style>" << HTMLLogger_css << "</style>\n";
+    *OS << "<script>" << HTMLLogger_js << "</script>\n";
+
----------------
gribozavr2 wrote:
> Now that we have an HTML template file, you could inline css and js into the file - if you like.
I find having them out of line much easier to navigate and edit.


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.html:19
+<header>Function</header>
+<div id="code"></div>
+<div id="cfg"></div>
----------------
gribozavr2 wrote:
> sammccall wrote:
> > gribozavr2 wrote:
> > > Could you add the file name and line number of the start location of the function?
> > > 
> > > It might be helpful not only for the reader (to confirm that they are debugging the right thing), but also when finding the right html file using grep.
> > Added filename to the top of the code, and line numbers to each line.
> > (This seems nice for display, but doesn't allow you to grep for `foo.cpp:42` - is that critical?)
> Imagine you're debugging an analysis on a relatively big file. How would you find the html file that corresponds to a function where the analysis is misbehaving? What if the function has a relatively common name?
Fair enough, added the function name and file:line to the <title> (in HTML rather than JS so they can be easily grepped)


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:45
+  }
+  root.hidden = false;
+  if (changed) {
----------------
gribozavr2 wrote:
> Should `hidden = false` be under `if (changed)`?
> 
> Or can it be the case that nothing changed, but we still need to show the element?
Moved it under the element, I'm not sure which is conceptually simpler.

It's not possible that we need to show but `!changed`, because either:
 - this is the first call (and changed is initialized to true), or
 - the previous call early-returned, in which case root.selection[key] was set to null for some key on the last call, and because we got here it was nonnull on this call, so again changed is true.


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:47-50
+    for (tmpl of root.getElementsByTagName('template')) {
+      // Clear previously rendered template contents.
+      while (tmpl.nextSibling && tmpl.nextSibling.inflated)
+        tmpl.parentNode.removeChild(tmpl.nextSibling);
----------------
gribozavr2 wrote:
> Would it make sense to move this removal logic into `inflate` itself?
> 
> (Of course the recursive part of `inflate` would stay as a separate function, `inflateImpl` or something.)
I think we end up with a 60 line function wrapped in a +4 line function, and the nesting causes more confusion than it solves. I don't think clearing the old content matches the name `inflate` either.

Happy to pull out `reinflate()` as a sibling if you think it helps - with one callsite I'm not sure it does.


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:124
+//   hover: a function from target to the class name to highlight
+//   bb: a function from target to the basic-block name to select (BB4)A
+//   elt: a function from target to the CFG element name to select (BB4.5)
----------------
gribozavr2 wrote:
> What's "A" in "(BB4)A"?
a typo :-(


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:184
+}
+function classSelector(cls) {
+  if (cls == null) return null;
----------------
gribozavr2 wrote:
> Could you add a doc comment that describes what cls is?
> 
> Specifically, I see that in the array case we assume that the dot is already present, and in the single-value case we prepend the dot - is that intentional?
> Could you add a doc comment that describes what cls is?
Added a comment.
I can also make this function a bit less magic at the cost of a bit more verbosity elsewhere (`id="foo"` => `id="foo" class="foo"` in the HTML, `e => e.id` => `e => [e.id]` in the JS).

> Specifically, I see that in the array case we assume that the dot is already present, and in the single-value case we prepend the dot - is that intentional?

In the array case we call `classSelector` on each element recursively, so the dot gets added there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591