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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I think the ` void FindTypes(const TypeQuery &query, TypeResults &results);` API looks like a reasonable compromise. I have a bunch of smaller questions inside, but otherwise I think this can work.



================
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);
 
----------------
Why is this not taking a TypeQuery that wraps a name?


================
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
----------------
copy&paste error?


================
Comment at: lldb/include/lldb/Core/ModuleList.h:350
   ///
-  /// \param[in] name
-  ///     The name of the type we are looking for.
-  ///
-  /// \param[in] max_matches
-  ///     Allow the number of matches to be limited to \a
-  ///     max_matches. Specify UINT32_MAX to get all possible matches.
-  ///
-  /// \param[out] types
-  ///     A type list gets populated with any matches.
+  /// \param[inout] match
+  ///     A type matching object that contains all of the details of the type
----------------
needs update now


================
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;
----------------
same question — why is this not a TypeQuery?


================
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.
----------------
declaration


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


================
Comment at: lldb/include/lldb/Symbol/Type.h:85
+  /// doing name lookups.
+  TypeQuery() = default;
+
----------------
Can we get rid of this now?


================
Comment at: lldb/include/lldb/Symbol/Type.h:231
+  /// Add a language family to the list of languages that should produce a match.
+  void AddLanguage(lldb::LanguageType language);
+
----------------
Would you mind sorting this closer to the constructors, so it doesn't appear in the middle of the list of functions that FindTypes implementation would use?

we could even wrap the setup / match functions in a doxygen group
/// \{
/// \}


================
Comment at: lldb/include/lldb/Symbol/Type.h:275
+  /// Clients can use this to populate the context manually.
+  llvm::SmallVector<CompilerContext, 4> &GetContextRef() { return m_context; }
+
----------------
Could this return an `ArrayRef<CompilerContext>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900