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