[Lldb-commits] [PATCH] D137900: Make only one function that needs to be implemented when searching for types.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Core/Module.h:435
+  ///   match: "b::a", "c::b::a", "d::b::a", "e::f::b::a".
+  lldb::TypeSP FindFirstType(ConstString type_name, bool exact_match);
 
----------------
aprantl wrote:
> Why is this not taking a TypeQuery that wraps a name?
This function doesn't need to exist as long as the "TypeSP TypeQuery::FindFirstType(Module)" function is ok to have around.


================
Comment at: lldb/include/lldb/Core/Module.h:442
   ///
-  /// \param searched_symbol_files
-  ///     Prevents one file from being visited multiple times.
-  void
-  FindTypes(llvm::ArrayRef<CompilerContext> pattern, LanguageSet languages,
-            llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
-            TypeMap &types);
-
-  lldb::TypeSP FindFirstType(const SymbolContext &sc, ConstString type_name,
-                             bool exact_match);
-
-  /// Find types by name that are in a namespace. This function is used by the
-  /// expression parser when searches need to happen in an exact namespace
-  /// scope.
+  /// \param[in] search_first
+  ///     If non-null, this module will be searched before any other
----------------
aprantl wrote:
> copy&paste error?
yep!


================
Comment at: lldb/include/lldb/Core/ModuleList.h:373
+  ///   match: "b::a", "c::b::a", "d::b::a", "e::f::b::a".
+  lldb::TypeSP FindFirstType(Module *search_first, ConstString type_name,
+                             bool exact_match) const;
----------------
aprantl wrote:
> same question — why is this not a TypeQuery?
Like in Module.h we can get rid of this as long as the convenience functions in TypeQuery::FindFirstType() are ok.


================
Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:90
+  /// \param context A valid vector of CompilerContext entries that describes
+  /// this delcaratiion context. The first entry in the vector is the parent of
+  /// the subsequent entry, so the top most entry is the global namespace.
----------------
aprantl wrote:
> declaration
will fix


================
Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+      llvm::SmallVectorImpl<lldb_private::CompilerContext> &context) const;
+
----------------
aprantl wrote:
> aprantl wrote:
> > Why can't this be a return value? The context objects are tiny.
> ping?
Will change!


================
Comment at: lldb/include/lldb/Symbol/Type.h:85
+  /// doing name lookups.
+  TypeQuery() = default;
+
----------------
aprantl wrote:
> Can we get rid of this now?
I believe so! I will change "default" to "delete".


================
Comment at: lldb/include/lldb/Symbol/Type.h:241
+  /// Add a language family to the list of languages that should produce a match.
+  void AddLanguage(lldb::LanguageType language);
+
----------------
aprantl wrote:
> Is this necessary, or could this be rolled into the constructor?
I will add it to the constructor.


================
Comment at: lldb/include/lldb/Symbol/Type.h:306
+  /// more complete are are used when lookup up types in a clang module's debug
+  /// information.
+  bool m_module_search = false;
----------------
aprantl wrote:
> This sentence is missing at least one word. (I also don't quite understand the purpose of this flag)
I will reword


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900