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