[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 have only two concerns left:

1. IMHO it would be better if `find_one` (which I think of as a search *input*) would live in TypeQuery instead of TypeResult. It feels wrong to use TypeResult to pass something *in*.
2. It's inconsistent that Module has a FindTypes(TypeQuery) method and TypeQuery has a Find(First)Type(Module) method.

Could we just move `find_one` into `TypeQuery` and get rid of the `Find` methods in `TypeQuery`.
Let me know if I'm misunderstanding something.



================
Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+      llvm::SmallVectorImpl<lldb_private::CompilerContext> &context) const;
+
----------------
clayborg wrote:
> clayborg wrote:
> > 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.
> change "preached" to "preset" above...
Thanks for checking — that makes sense.


================
Comment at: lldb/source/Symbol/Type.cpp:149
+  module->FindTypes(*this, results);
+  return results.GetTypeMap().FirstType();
+}
----------------
IMHO it would be better if `find_one` (which I think of as a search *input*) would live in TypeQuery instead of TypeResult. It feels wrong to use TypeResult to pass something *in*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900