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