[Lldb-commits] [PATCH] D51453: Refactor BreakpointResolver::SetSCMatchesByLine()

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Very nice! Do we have a way of verifying the performance gain?



================
Comment at: source/Breakpoint/BreakpointResolver.cpp:183
                                             llvm::StringRef log_ident) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
-
-  while (sc_list.GetSize() > 0) {
-    SymbolContextList tmp_sc_list;
-    unsigned current_idx = 0;
-    SymbolContext sc;
-    bool first_entry = true;
-
-    FileSpec match_file_spec;
-    FileSpec match_original_file_spec;
-    uint32_t closest_line_number = UINT32_MAX;
-
-    // Pull out the first entry, and all the others that match its file spec,
-    // and stuff them in the tmp list.
-    while (current_idx < sc_list.GetSize()) {
-      bool matches;
-
-      sc_list.GetContextAtIndex(current_idx, sc);
-      if (first_entry) {
-        match_file_spec = sc.line_entry.file;
-        match_original_file_spec = sc.line_entry.original_file;
-        matches = true;
-        first_entry = false;
-      } else
-        matches = ((sc.line_entry.file == match_file_spec) ||
-                   (sc.line_entry.original_file == match_original_file_spec));
-
-      if (matches) {
-        tmp_sc_list.Append(sc);
-        sc_list.RemoveContextAtIndex(current_idx);
-
-        // ResolveSymbolContext will always return a number that is >= the line
-        // number you pass in. So the smaller line number is always better.
-        if (sc.line_entry.line < closest_line_number)
-          closest_line_number = sc.line_entry.line;
-      } else
-        current_idx++;
+  llvm::SmallVector<SymbolContext, 16> all_scs;
+  for (unsigned i = 0; i < sc_list.GetSize(); ++i)
----------------
The number `16` and `8` below where do they come from?


================
Comment at: source/Breakpoint/BreakpointResolver.cpp:184
+  llvm::SmallVector<SymbolContext, 16> all_scs;
+  for (unsigned i = 0; i < sc_list.GetSize(); ++i)
+    all_scs.push_back(sc_list[i]);
----------------
`GetSize()` returns `uint32_t` we should match the type.


================
Comment at: source/Breakpoint/BreakpointResolver.cpp:214
+    std::sort(worklist_begin, worklist_end,
+              [&](const SymbolContext &a, const SymbolContext &b) {
+                return a.line_entry.range.GetBaseAddress().GetFileAddress() <
----------------
I don't think you need a capture here.


================
Comment at: source/Breakpoint/BreakpointResolver.cpp:228
+          std::remove_if(std::next(first), worklist_end,
+                         [&](const SymbolContext &sc) {
+                           return blocks_with_breakpoints.count(sc.block);
----------------
Although in the other lambdas being explicit with the capture list might reduce readability here we are only capturing `blocks_with_breakpoints`


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51453