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

Mon Mar 27 09:18:14 PDT 2023

gribozavr2 added inline comments.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:34
+// the data into <template> tags embedded in the HTML. (see inflate() in JS).
+// 
+// SELECTION
----------------

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:128
+    *OS << "<style>" << HTMLLogger_css << "</style>\n";
+    *OS << "<script>" << HTMLLogger_js << "</script>\n";
+
----------------
Now that we have an HTML template file, you could inline css and js into the file - if you like.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.html:1
+<!doctype html>
+<html>
----------------
Please add a copyright statement.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:11
+//
+// For example, if we the selection is {bb=BB4, elt=BB4.6 iter=BB4:2}:
+//   - show the "block" and "element" sections
----------------

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:11
+//
+// For example, if we the selection is {bb=BB4, elt=BB4.6 iter=BB4:2}:
+//   - show the "block" and "element" sections
----------------
gribozavr2 wrote:
> 
"if we the selection is" - parse error :)

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:45
+  }
+  root.hidden = false;
+  if (changed) {
----------------
Should `hidden = false` be under `if (changed)`?

Or can it be the case that nothing changed, but we still need to show the element?

================
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);
----------------
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.)

================
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)
----------------
What's "A" in "(BB4)A"?

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:184
+}
+function classSelector(cls) {
+  if (cls == null) return null;
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591