[Lldb-commits] [PATCH] D100740: json

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/include/lldb/Symbol/LineEntry.h:94-104
+  /// \param[in] compare_addresses
+  ///     If \b true, then the addresses of the LineEntry objects will be
+  ///     compared, otherwise only the line, column and file names must match.
+  ///
   /// \return
   ///     -1 if lhs < rhs
   ///     0 if lhs == rhs
----------------
I would leave the compare function alone and create a new one that just compares the file and line only. Why? The compare function is comparing a lot more: LineEntry::is_terminal_entry, LineEntry::column. So make a new function, and we don't need to return a -1, 0, or 1, but a bool:

```
static bool FileAndLineMatches(const LineEntry &lhs, const LineEntry &rhs);
```

Or just compare the file and line manually where this was being called.


================
Comment at: lldb/include/lldb/Target/Trace.h:208-216
+  /// Helper structure for \a TraverseInstructionsWithSymbolInfo.
+  struct InstructionSymbolInfo {
+    SymbolContext sc;
+    Address address;
+    lldb::addr_t load_address;
+    lldb::DisassemblerSP disassembler;
+    lldb::InstructionSP instruction;
----------------
Does this need to be exposed in the Trace.h file? Seems like this could be declared in the Trace.cpp file


================
Comment at: lldb/include/lldb/Target/Trace.h:218
+
+  /// Similar to \a TraverseInstructions byt the callback receives an \a
+  /// InstructionSymbolInfo object with symbol information for the given
----------------
s/byt/but/


================
Comment at: lldb/include/lldb/Target/Trace.h:219-234
+  /// InstructionSymbolInfo object with symbol information for the given
+  /// instruction, calculated efficiently.
+  ///
+  /// \param[in] symbol_scope
+  ///     If not \b 0, then the \a InstructionSymbolInfo will have its
+  ///     SymbolContext calculated up to that level.
+  ///
----------------
Can we put a static function into Trace.cpp so this isn't needed in the Trace.h file?


================
Comment at: lldb/source/Target/Trace.cpp:114-115
+static bool
+IsSameInstructionSymbolContext(Optional<Trace::InstructionSymbolInfo> prev_insn,
+                               Trace::InstructionSymbolInfo &insn) {
+  if (!prev_insn)
----------------
Remove the "Optional<Trace::InstructionSymbolInfo>" for "prev_insn and just check it prior to calling and make params const






================
Comment at: lldb/source/Target/Trace.cpp:118
+    return false;
+
+  // This custom LineEntry validator is neded because some line_entries have
----------------
we should compare the function/symbol first, then do the line entries.


================
Comment at: lldb/source/Target/Trace.cpp:127
+  // line entry checks
+  if (is_line_entry_valid(insn.sc.line_entry) ^
+      is_line_entry_valid(prev_insn->sc.line_entry))
----------------
use != instead of ^ to make the code easier to deduce.


================
Comment at: lldb/source/Target/Trace.cpp:132
+      is_line_entry_valid(prev_insn->sc.line_entry))
+    return LineEntry::Compare(insn.sc.line_entry, prev_insn->sc.line_entry,
+                              /*compare_addresses*/ false) == 0;
----------------
call LjneEntry::FileAndLineMatches(...) or just manually compare the file and line only.


================
Comment at: lldb/source/Target/Trace.cpp:135-139
+  // function checks
+  if ((insn.sc.function != nullptr) ^ (prev_insn->sc.function != nullptr))
+    return false;
+  if (insn.sc.function != nullptr && prev_insn->sc.function != nullptr)
+    return insn.sc.function == prev_insn->sc.function;
----------------
Don't we want to check the function first before the line entry? What if the function changes but we have the same file and line, like a vector:23 from STL...

Also you can probably just make this code much much simpler



================
Comment at: lldb/source/Target/Trace.cpp:142-145
+  if ((insn.sc.symbol != nullptr) ^ (prev_insn->sc.symbol != nullptr))
+    return false;
+  if (insn.sc.symbol != nullptr && prev_insn->sc.symbol != nullptr)
+    return insn.sc.symbol == prev_insn->sc.symbol;
----------------
Ditto, simplify:




================
Comment at: lldb/source/Target/Trace.cpp:148
+  // module checks
+  return insn.sc.module_sp == prev_insn->sc.module_sp;
+}
----------------
No need to do a module check as the module owns the function or symbol and they won't match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100740