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