[Lldb-commits] [PATCH] D71022: [lldb] NFC: less nesting in SearchFilter.cpp

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk requested review of this revision.
kwk marked 2 inline comments as done.
kwk added a comment.

Addressed all review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71022



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71022: [lldb] NFC: less nesting in SearchFilter.cpp

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 232494.
kwk added a comment.

- Review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71022

Files:
  lldb/source/Core/SearchFilter.cpp

Index: lldb/source/Core/SearchFilter.cpp
===
--- lldb/source/Core/SearchFilter.cpp
+++ lldb/source/Core/SearchFilter.cpp
@@ -208,10 +208,12 @@
 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;
+  }
+  
+  DoModuleIteration(empty_sc, searcher);
 }
 
 void SearchFilter::SearchInModuleList(Searcher , ModuleList ) {
@@ -221,20 +223,20 @@
 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 guard(modules.GetMutex());
-const size_t numModules = modules.GetSize();
-
-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;
-  }
-}
+return;
+  }
+
+  std::lock_guard guard(modules.GetMutex());
+  const size_t numModules = modules.GetSize();
+
+  for (size_t i = 0; i < numModules; i++) {
+ModuleSP module_sp(modules.GetModuleAtIndexUnlocked(i));
+if (!ModulePasses(module_sp))
+  continue;
+if (DoModuleIteration(module_sp, searcher) == Searcher::eCallbackReturnStop)
+  return;
   }
 }
 
@@ -248,45 +250,47 @@
 Searcher::CallbackReturn
 SearchFilter::DoModuleIteration(const SymbolContext ,
 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);
-  }
+  if (searcher.GetDepth() < lldb::eSearchDepthModule)
+return Searcher::eCallbackReturnContinue;
+
+  if (context.module_sp) {
+if (searcher.GetDepth() != lldb::eSearchDepthModule)
+  return DoCUIteration(context.module_sp, context, searcher);
+
+SymbolContext matchingContext(context.module_sp.get());
+searcher.SearchCallback(*this, matchingContext, nullptr);
+return Searcher::eCallbackReturnContinue;
+  }
+
+  const ModuleList _images = m_target_sp->GetImages();
+  std::lock_guard guard(target_images.GetMutex());
+
+  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 {
-  const ModuleList _images = m_target_sp->GetImages();
-  std::lock_guard guard(target_images.GetMutex());
-
-  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;
-}
-  }
+  Searcher::CallbackReturn shouldContinue =
+  

[Lldb-commits] [PATCH] D71022: [lldb] NFC: less nesting in SearchFilter.cpp

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

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




Comment at: lldb/source/Core/SearchFilter.cpp:777
 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) !=

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71022



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71022: [lldb] NFC: less nesting in SearchFilter.cpp

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Core/SearchFilter.cpp:625
  modules_array);
   FileSpecList modules;
+  if (!success)

I'd move this down and use 
```
  if (!success)
return 
std::make_shared(target.shared_from_this(),{});

```
to make it clear the list of modules is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71022



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71022: [lldb] NFC: less nesting in SearchFilter.cpp

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk created this revision.
Herald added a reviewer: jdoerfert.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
kwk edited the summary of this revision.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71022

Files:
  lldb/source/Core/SearchFilter.cpp

Index: lldb/source/Core/SearchFilter.cpp
===
--- lldb/source/Core/SearchFilter.cpp
+++ lldb/source/Core/SearchFilter.cpp
@@ -208,10 +208,12 @@
 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;
+  }
+  
+  DoModuleIteration(empty_sc, searcher);
 }
 
 void SearchFilter::SearchInModuleList(Searcher , ModuleList ) {
@@ -221,20 +223,20 @@
 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 guard(modules.GetMutex());
-const size_t numModules = modules.GetSize();
-
-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;
-  }
-}
+return;
+  }
+
+  std::lock_guard guard(modules.GetMutex());
+  const size_t numModules = modules.GetSize();
+
+  for (size_t i = 0; i < numModules; i++) {
+ModuleSP module_sp(modules.GetModuleAtIndexUnlocked(i));
+if (!ModulePasses(module_sp))
+  continue;
+if (DoModuleIteration(module_sp, searcher) == Searcher::eCallbackReturnStop)
+  return;
   }
 }
 
@@ -248,45 +250,47 @@
 Searcher::CallbackReturn
 SearchFilter::DoModuleIteration(const SymbolContext ,
 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);
-  }
+  if (searcher.GetDepth() < lldb::eSearchDepthModule)
+return Searcher::eCallbackReturnContinue;
+
+  if (context.module_sp) {
+if (searcher.GetDepth() != lldb::eSearchDepthModule)
+  return DoCUIteration(context.module_sp, context, searcher);
+
+SymbolContext matchingContext(context.module_sp.get());
+searcher.SearchCallback(*this, matchingContext, nullptr);
+return Searcher::eCallbackReturnContinue;
+  }
+
+  const ModuleList _images = m_target_sp->GetImages();
+  std::lock_guard guard(target_images.GetMutex());
+
+  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 {
-  const ModuleList _images = m_target_sp->GetImages();
-  std::lock_guard guard(target_images.GetMutex());
-
-  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 ==