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

Tue Apr 11 07:23:12 PDT 2023

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.

================
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);
----------------
sammccall wrote:
> 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.
> Happy to pull out reinflate() as a sibling if you think it helps - with one callsite I'm not sure it does.

I'd prefer that. The reason is that you can't correctly call inflate without first deleting the previously expanded nodes (even though there's only one call site.) And that suggests to me that the callee should be responsible for that part of the work as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591