[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:
> clayborg wrote:
> > 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.
> It would be good to have some consistency in the interface:
> - Either TypeQuery provides Find* methods that take a Module 
> - or Module provides Find* methods that take a TypeQuery.
> 
> Why does this interface exist instead of having a "FindFirstOnly" flag in TypeQuery? Is it because of the overhead of having to dig the type out of a TypeMap?
> 
> Otherwise we could just have one method
> 
> Module::FindTypes(const TypeQuery &query, TypeResults &results);
> 
> that can be called like this
> ```
> module_sp->FindTypes(TypeQuery::createFindFirstByName("name", e_exact_match), result);
> TypeSP type = result.GetTypeAtIndex(0); /// better have a special method for this too
> ```
See what you think of the current code and let me know any changes you would like to see.


================
Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+      llvm::SmallVectorImpl<lldb_private::CompilerContext> &context) const;
+
----------------
clayborg wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Why can't this be a return value? The context objects are tiny.
> > ping?
> Will change!
I looked around LLVM code and there are many accessors that use this technique. Otherwise we need to enforce an exact SmallVector type as the return code and we can't use "llvm::SmallVectorImpl<lldb_private::CompilerContext>". I can change this to return a "llvm::SmallVector<CompilerContext, 4>" if needed? But that locks us into a specific small vector with a specific size of preached items.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900