wallace added inline comments.
================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:19 +#include "lldb/lldb-types.h" +#include "llvm/Analysis/ModuleSummaryAnalysis.h" +#include "llvm/Support/Error.h" ---------------- this doesn't seem relevant ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:22 +#include "llvm/Support/JSON.h" +#include <fstream> +#include <sstream> ---------------- leave a black line before this ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:31 + +llvm::Error TraceIntelPTSessionFileSaver::SaveToDisk( + lldb::ProcessSP &process_sp, TraceIntelPT &trace_ipt, FileSpec directory) { ---------------- Better rename this class to TraceIntelPTSessionSaver, because this is not just working with a file, it's instead dealing with a directory of many files ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:56 + +void TraceIntelPTSessionFileSaver::WriteIntelPTSchemaToFile( + const JSONTraceIntelPTSchema &json_trace_intel_pt_schema, ---------------- To make these useful functions more generic so that they can be reused by other Trace technologies, you can create a file TraceSessionSaver.h/cpp in Plugins/Trace/common. Then you can move this function to that file an call it WriteTraceSession(llvm::Value session_json, FileSpec directory) ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:87 +llvm::Expected<JSONTraceSessionBase> +TraceIntelPTSessionFileSaver::BuildProcessesSection(lldb::ProcessSP &process_sp, + TraceIntelPT &trace_ipt, ---------------- you could move BuildProcessesSection, BuildThreadsSection and BuildModulesSection to TraceSessionSaver.h/cpp if you provide a callback that receives a thread_id and returns a raw trace. Then almost all the code would be reusable by other Trace plug-ins CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107669/new/ https://reviews.llvm.org/D107669 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits