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