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

Wed Mar 22 15:58:15 PDT 2023

gribozavr2 added inline comments.

================
Comment at: clang/include/clang/Analysis/FlowSensitive/Logger.h:34
   static std::unique_ptr<Logger> textual(llvm::raw_ostream &);
+  // A logger that builds an HTML UI to inspect the analysis results.
+  // One file is written under the specified dir per analyzed function.
----------------
Please use triple slash for doc comments.

================
Comment at: clang/lib/Analysis/FlowSensitive/CMakeLists.txt:22
+add_custom_command(OUTPUT HTMLLogger.inc
+  COMMAND "${Python3_EXECUTABLE}" bundle_resources.py 
+  ${CMAKE_CURRENT_BINARY_DIR}/HTMLLogger.inc
----------------
Is this the right location for the Python script? Aren't they normally under `llvm-project.git/clang/utils`?

================
Comment at: clang/lib/Analysis/FlowSensitive/CMakeLists.txt:31
+add_dependencies(clangAnalysisFlowSensitive clangAnalysisFlowSensitiveResources)
\ No newline at end of file

----------------
Please add a blank line.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:14
+// Static assets (HTMLLogger.js, HTMLLogger.css) and SVG graphs etc are embedded
+// so this file is self-contained.
+//
----------------

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:119
+    extern const char HTMLLogger_js[];
+    extern const char HTMLLogger_css[];
+    *OS << "<style>" << HTMLLogger_css << "</style>\n";
----------------
Why not include the generated header before the class definition to avoid the need for forward declarations?

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:123
+  }
+  // between beginAnalysis() and endAnalysis() we write all the dumps for
+  // particular analysis points into <template>s inside <head>.
----------------

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:212
+      *OS << "<h2>Log messages</h2>\n<pre>";
+      *OS << "</pre>\n";
+    }
----------------
Did you mean to append ContextLogs here? (and then clear them)

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.css:1
+html { font-family: sans-serif; }
+body { margin: 0; display: flex; justify-content: left; }
----------------
Please add a copyright comment.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:1
+// Based on selected objects, hide/show sections & populate data from templates.
+//
----------------
Please add a copyright comment.

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:75
+  var root = document.querySelector(rootSelector);
+  console.log(root, rootSelector);
+  for (event of ['mouseout', 'mousemove', 'click'])
----------------
Remove debug logging?

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:111
+  document.querySelectorAll(query).forEach(elt => elt.classList.add(cls));
+  console.log(cls, "=>", query);
+}
----------------
Remove debug logging?

================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.js:128
+  // hex values to subtract from fff to get a base color
+  options = [0x001, 0x010, 0x100, 0x011, 0x101, 0x110, 0x111];
+  function color(hex) {
----------------
Or was the order intentional?

================
Comment at: clang/lib/Analysis/FlowSensitive/bundle_resources.py:1
+#!/usr/bin/env python3
+# Simple bundler of files into string constants.
----------------
Please add a copyright comment.

================
Comment at: clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp:93
 )cpp";
+  static std::vector<std::string> Args = {
+      "-fsyntax-only", "-fno-delayed-template-parsing", "-std=c++17"};
----------------
const?

================
Comment at: clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp:176
+  EXPECT_THAT(Logs[0], HasSubstr("<template id='template-B3:1_B3.1'"))
+      << "has analysis point state";
+}
----------------
Please also verify that we include the custom log messages and pretty-printed lattice elements.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146591