[lldb] NFC: less nesting in SearchFilter.cpp

[kwk]

Summary

I was working on SearchFilter.cpp and felt it a bit too complex in some cases in terms of nesting and logic flow.

[JDevlieghere]

lldb/source/Core/SearchFilter.cpp

625

I'd move this down and use

if (!success)
  return std::make_shared<SearchFilterByModuleList>(target.shared_from_this(),{});

to make it clear the list of modules is empty.

[teemperor]

I have one minor request (see inline comments), but otherwise LGTM, thanks!

lldb/source/Core/SearchFilter.cpp

777

Simplify with de Morgan (or maybe we could get rid of the no_modules_in_filter as this just seems to be an "optimization" to avoid the FindFileIndex function call, but just getting rid of the !(...) is fine).

[kwk]

• Review comments

[kwk]

Addressed all review comments.

lldb/source/Core/SearchFilter.cpp

 
 void SearchFilter::Search(Searcher &searcher) {
   SymbolContext empty_sc;
 
   if (!m_target_sp)
     return;
   empty_sc.target_sp = m_target_sp;
 
-  if (searcher.GetDepth() == lldb::eSearchDepthTarget)
+  if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
-  else
-    DoModuleIteration(empty_sc, searcher);
+    return;
+  }
+  
 }
 
 void SearchFilter::SearchInModuleList(Searcher &searcher, ModuleList &modules) {
   SymbolContext empty_sc;
 
   if (!m_target_sp)
     return;
   empty_sc.target_sp = m_target_sp;
 
-  if (searcher.GetDepth() == lldb::eSearchDepthTarget)
+  if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
-  else {
-    std::lock_guard<std::recursive_mutex> guard(modules.GetMutex());
-    const size_t numModules = modules.GetSize();
+    return;
+  }
+
 
-    for (size_t i = 0; i < numModules; i++) {
-      ModuleSP module_sp(modules.GetModuleAtIndexUnlocked(i));
-      if (ModulePasses(module_sp)) {
-        if (DoModuleIteration(module_sp, searcher) ==
-            Searcher::eCallbackReturnStop)
-          return;
-      }
-    }
-  }
-}
+    if (!ModulePasses(module_sp))
+      continue;
+    if (DoModuleIteration(module_sp, searcher) == Searcher::eCallbackReturnStop)
 
 Searcher::CallbackReturn
 SearchFilter::DoModuleIteration(const lldb::ModuleSP &module_sp,
                                 Searcher &searcher) {
   SymbolContext matchingContext(m_target_sp, module_sp);
   return DoModuleIteration(matchingContext, searcher);
 }
 
 Searcher::CallbackReturn
 SearchFilter::DoModuleIteration(const SymbolContext &context,
                                 Searcher &searcher) {
-  if (searcher.GetDepth() >= lldb::eSearchDepthModule) {
-    if (context.module_sp) {
-      if (searcher.GetDepth() == lldb::eSearchDepthModule) {
-        SymbolContext matchingContext(context.module_sp.get());
-        searcher.SearchCallback(*this, matchingContext, nullptr);
-      } else {
-        return DoCUIteration(context.module_sp, context, searcher);
-      }
-    } else {
-      const ModuleList &target_images = m_target_sp->GetImages();
-      std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
+  if (searcher.GetDepth() < lldb::eSearchDepthModule)
+    return Searcher::eCallbackReturnContinue;
+
+    if (searcher.GetDepth() != lldb::eSearchDepthModule)
+      return DoCUIteration(context.module_sp, context, searcher);
+
+    return Searcher::eCallbackReturnContinue;
+
 
-      size_t n_modules = target_images.GetSize();
-      for (size_t i = 0; i < n_modules; i++) {
-        // If this is the last level supplied, then call the callback directly,
-        // otherwise descend.
-        ModuleSP module_sp(target_images.GetModuleAtIndexUnlocked(i));
-        if (!ModulePasses(module_sp))
-          continue;
 
-        if (searcher.GetDepth() == lldb::eSearchDepthModule) {
-          SymbolContext matchingContext(m_target_sp, module_sp);
 
-          Searcher::CallbackReturn shouldContinue =
-              searcher.SearchCallback(*this, matchingContext, nullptr);
-          if (shouldContinue == Searcher::eCallbackReturnStop ||
-              shouldContinue == Searcher::eCallbackReturnPop)
-            return shouldContinue;
-        } else {
-          Searcher::CallbackReturn shouldContinue =
-              DoCUIteration(module_sp, context, searcher);
-          if (shouldContinue == Searcher::eCallbackReturnStop)
-            return shouldContinue;
-          else if (shouldContinue == Searcher::eCallbackReturnPop)
-            continue;
-        }
-      }
-    }
-  }
+
   return Searcher::eCallbackReturnContinue;
 }
 
 Searcher::CallbackReturn
 SearchFilter::DoCUIteration(const ModuleSP &module_sp,
                             const SymbolContext &context, Searcher &searcher) {
   Searcher::CallbackReturn shouldContinue;
-  if (context.comp_unit == nullptr) {
-    const size_t num_comp_units = module_sp->GetNumCompileUnits();
-    for (size_t i = 0; i < num_comp_units; i++) {
-      CompUnitSP cu_sp(module_sp->GetCompileUnitAtIndex(i));
-      if (cu_sp) {
-        if (!CompUnitPasses(*(cu_sp.get())))
-          continue;
+  if (context.comp_unit != nullptr) {
+    if (CompUnitPasses(*context.comp_unit)) {
+      SymbolContext matchingContext(m_target_sp, module_sp, context.comp_unit);
+      return searcher.SearchCallback(*this, matchingContext, nullptr);
+    }
+    return Searcher::eCallbackReturnContinue;
+  }
+
+    if (!cu_sp)
+      continue;
 
-        if (searcher.GetDepth() == lldb::eSearchDepthCompUnit) {
-          SymbolContext matchingContext(m_target_sp, module_sp, cu_sp.get());
 
-          shouldContinue =
-              searcher.SearchCallback(*this, matchingContext, nullptr);
+      shouldContinue = searcher.SearchCallback(*this, matchingContext, nullptr);
 
-          if (shouldContinue == Searcher::eCallbackReturnPop)
-            return Searcher::eCallbackReturnContinue;
-          else if (shouldContinue == Searcher::eCallbackReturnStop)
-            return shouldContinue;
-        } else {
-          // First make sure this compile unit's functions are parsed
-          // since CompUnit::ForeachFunction only iterates over already
-          // parsed functions.
-          SymbolFile *sym_file = module_sp->GetSymbolFile();
-          if (!sym_file)
-            continue;
-          if (!sym_file->ParseFunctions(*cu_sp))
-            continue;
-          // If we got any functions, use ForeachFunction to do the iteration.
-          cu_sp->ForeachFunction([&](const FunctionSP &func_sp) {
-            if (!FunctionPasses(*func_sp.get()))
-              return false; // Didn't pass the filter, just keep going.
-            if (searcher.GetDepth() == lldb::eSearchDepthFunction) {
-              SymbolContext matchingContext(m_target_sp, module_sp, 
-                                            cu_sp.get(), func_sp.get());
-              shouldContinue =
-                  searcher.SearchCallback(*this, matchingContext, nullptr);
-            } else {
-              shouldContinue = DoFunctionIteration(func_sp.get(), context, 
-                                                   searcher);
-            }
-            return shouldContinue != Searcher::eCallbackReturnContinue;
-          });
-        }
-      }
-    }
-  } else {
-    if (CompUnitPasses(*context.comp_unit)) {
-      SymbolContext matchingContext(m_target_sp, module_sp, context.comp_unit);
-      return searcher.SearchCallback(*this, matchingContext, nullptr);
-    }
-  }
+      continue;
+    }
+
+        SymbolContext matchingContext(m_target_sp, module_sp, cu_sp.get(),
+                                      func_sp.get());
+        shouldContinue = DoFunctionIteration(func_sp.get(), context, searcher);
   return Searcher::eCallbackReturnContinue;
 }
 
 Searcher::CallbackReturn SearchFilter::DoFunctionIteration(
     Function *function, const SymbolContext &context, Searcher &searcher) {
   // FIXME: Implement...
   return Searcher::eCallbackReturnContinue;
 }

 }
 
 bool SearchFilterForUnconstrainedSearches::ModulePasses(
     const lldb::ModuleSP &module_sp) {
   if (!module_sp)
     return true;
   else if (m_target_sp->ModuleIsExcludedForUnconstrainedSearches(module_sp))
     return false;
-  else
-    return true;
 }
 
 lldb::SearchFilterSP SearchFilterForUnconstrainedSearches::DoCopyForBreakpoint(
     Breakpoint &breakpoint) {
   return std::make_shared<SearchFilterForUnconstrainedSearches>(*this);
 }
 
 //  SearchFilterByModule:
void SearchFilterByModuleList::Search(Searcher &searcher) {
   // find the ones that match the file name.
 
   const ModuleList &target_modules = m_target_sp->GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
 
   const size_t num_modules = target_modules.GetSize();
   for (size_t i = 0; i < num_modules; i++) {
     Module *module = target_modules.GetModulePointerAtIndexUnlocked(i);
-    if (m_module_spec_list.FindFileIndex(0, module->GetFileSpec(), false) !=
-        UINT32_MAX) {
-      SymbolContext matchingContext(m_target_sp, module->shared_from_this());
-      Searcher::CallbackReturn shouldContinue;
+    if (m_module_spec_list.FindFileIndex(0, module->GetFileSpec(), false) ==
+        UINT32_MAX)
+      continue;
 
-      shouldContinue = DoModuleIteration(matchingContext, searcher);
-      if (shouldContinue == Searcher::eCallbackReturnStop)
-        return;
-    }
-  }
-}
 
 void SearchFilterByModuleList::GetDescription(Stream *s) {
   size_t num_modules = m_module_spec_list.GetSize();
   if (num_modules == 1) {
     s->Printf(", module = ");
     s->PutCString(
         m_module_spec_list.GetFileSpecAtIndex(0).GetFilename().AsCString(
             "<Unknown>"));
-  } else {
-    s->Printf(", modules(%" PRIu64 ") = ", (uint64_t)num_modules);
-    for (size_t i = 0; i < num_modules; i++) {
-      s->PutCString(
-          m_module_spec_list.GetFileSpecAtIndex(i).GetFilename().AsCString(
-              "<Unknown>"));
-      if (i != num_modules - 1)
-        s->PutCString(", ");
-    }
-  }
-}
+    return;
+  }
+
 
 uint32_t SearchFilterByModuleList::GetFilterRequiredItems() {
   return eSymbolContextModule;
 }
 
 void SearchFilterByModuleList::Dump(Stream *s) const {}
 
 lldb::SearchFilterSP
 SearchFilterByModuleList::DoCopyForBreakpoint(Breakpoint &breakpoint) {
   return std::make_shared<SearchFilterByModuleList>(*this);
 }
 
 SearchFilterSP SearchFilterByModuleList::CreateFromStructuredData(
     Target &target, const StructuredData::Dictionary &data_dict,
     Status &error) {
   StructuredData::Array *modules_array;
   bool success = data_dict.GetValueForKeyAsArray(GetKey(OptionNames::ModList),
                                                  modules_array);
   FileSpecList modules;
-  if (success) {
-    size_t num_modules = modules_array->GetSize();
-    for (size_t i = 0; i < num_modules; i++) {
-      llvm::StringRef module;
-      success = modules_array->GetItemAtIndexAsString(i, module);
-      if (!success) {
-        error.SetErrorStringWithFormat(
-            "SFBM::CFSD: filter module item %zu not a string.", i);
-        return nullptr;
-      }
-      modules.EmplaceBack(module);
-    }
-  }
-
+  if (!success)
+    return std::make_shared<SearchFilterByModuleList>(target.shared_from_this(),
+                                                      modules);
   return std::make_shared<SearchFilterByModuleList>(target.shared_from_this(),
                                                     modules);
 }
 
 void SearchFilterByModuleList::SerializeUnwrapped(
     StructuredData::DictionarySP &options_dict_sp) {
   SerializeFileSpecList(options_dict_sp, OptionNames::ModList,
                         m_module_spec_list);
lldb::SearchFilterSP SearchFilterByModuleListAndCU::CreateFromStructuredData(
 
   size_t num_cus = cus_array->GetSize();
   FileSpecList cus;
   for (size_t i = 0; i < num_cus; i++) {
     llvm::StringRef cu;
     success = cus_array->GetItemAtIndexAsString(i, cu);
     if (!success) {
       error.SetErrorStringWithFormat(
-          "SFBM::CFSD: filter cu item %zu not a string.", i);
+          "SFBM::CFSD: filter CU item %zu not a string.", i);
       return nullptr;
     }
     cus.EmplaceBack(cu);
   }
 
   return std::make_shared<SearchFilterByModuleListAndCU>(
       target.shared_from_this(), modules, cus);
 }

 
 bool SearchFilterByModuleListAndCU::CompUnitPasses(FileSpec &fileSpec) {
   return m_cu_spec_list.FindFileIndex(0, fileSpec, false) != UINT32_MAX;
 }
 
 bool SearchFilterByModuleListAndCU::CompUnitPasses(CompileUnit &compUnit) {
   bool in_cu_list = m_cu_spec_list.FindFileIndex(0, compUnit.GetPrimaryFile(),
                                                  false) != UINT32_MAX;
-  if (in_cu_list) {
-    ModuleSP module_sp(compUnit.GetModule());
-    if (module_sp) {
-      bool module_passes = SearchFilterByModuleList::ModulePasses(module_sp);
-      return module_passes;
-    } else
-      return true;
-  } else
-    return false;
+  if (!in_cu_list)
+    return false;
+
+  if (!module_sp)
+
+  return SearchFilterByModuleList::ModulePasses(module_sp);
 }
 
 void SearchFilterByModuleListAndCU::Search(Searcher &searcher) {
   if (!m_target_sp)
     return;
 
   if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     SymbolContext empty_sc;
     empty_sc.target_sp = m_target_sp;
     searcher.SearchCallback(*this, empty_sc, nullptr);
   }
 
   // If the module file spec is a full path, then we can just find the one
   // filespec that passes.  Otherwise, we need to go through all modules and
   // find the ones that match the file name.
 
   ModuleList matching_modules;
   const ModuleList &target_images = m_target_sp->GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
 
   const size_t num_modules = target_images.GetSize();
   bool no_modules_in_filter = m_module_spec_list.GetSize() == 0;
   for (size_t i = 0; i < num_modules; i++) {
     lldb::ModuleSP module_sp = target_images.GetModuleAtIndexUnlocked(i);
-    if (no_modules_in_filter ||
+    if (!(no_modules_in_filter ||
         m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) !=
-            UINT32_MAX) {
-      SymbolContext matchingContext(m_target_sp, module_sp);
-      Searcher::CallbackReturn shouldContinue;
+            UINT32_MAX))
+      continue;
+
 
-      if (searcher.GetDepth() == lldb::eSearchDepthModule) {
-        shouldContinue = DoModuleIteration(matchingContext, searcher);
-        if (shouldContinue == Searcher::eCallbackReturnStop)
-          return;
-      } else {
-        const size_t num_cu = module_sp->GetNumCompileUnits();
-        for (size_t cu_idx = 0; cu_idx < num_cu; cu_idx++) {
-          CompUnitSP cu_sp = module_sp->GetCompileUnitAtIndex(cu_idx);
-          matchingContext.comp_unit = cu_sp.get();
-          if (matchingContext.comp_unit) {
-            if (m_cu_spec_list.FindFileIndex(
-                    0, matchingContext.comp_unit->GetPrimaryFile(), false) !=
-                UINT32_MAX) {
-              shouldContinue =
-                  DoCUIteration(module_sp, matchingContext, searcher);
-              if (shouldContinue == Searcher::eCallbackReturnStop)
-                return;
-            }
-          }
-        }
-      }
-    }
-  }
-}
+      continue;
+    }
+
+      if (!matchingContext.comp_unit)
+        continue;
+              0, matchingContext.comp_unit->GetPrimaryFile(), false) ==
+          UINT32_MAX)
+        continue;
+      shouldContinue = DoCUIteration(module_sp, matchingContext, searcher);
 
 void SearchFilterByModuleListAndCU::GetDescription(Stream *s) {
   size_t num_modules = m_module_spec_list.GetSize();
   if (num_modules == 1) {
     s->Printf(", module = ");
     s->PutCString(
         m_module_spec_list.GetFileSpecAtIndex(0).GetFilename().AsCString(
             "<Unknown>"));