[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 a comment.

In D137900#3938759 <https://reviews.llvm.org/D137900#3938759>, @aprantl wrote:

> I still have a few more questions about the interface design here. May main goal is to eliminate as much mutable state and objects being passed by reference as makes sense (and no more :-).
> In that vain I'm trying to get rid of all Set...() methods and all places where we pass a mutable reference to something. The accumulating TypeMap may be a good example where that isn't possible, though I wonder how common global exhaustive searches are and if we usually wouldn't stop in most cases after we found the first match?

We look up many matches in the expression parser when we have a context to use for the lookup, and then we just have a base type name and we must return all of the results. Also people might want to lookup all of the types for a qualified name. So multiple matches are needed and need to span across modules so that expression lookups can get all of the information they need.

> I appreciate splitting it up into
>
> `FindTypes(match, results);`
>
> My hope was for `match` to be completely immutable and passed by value (const reference if need be), so I wonder if the  m_searched_symbol_files dictionary shouldn't be part of the result object?
> Are all the setters really necessary? Could we just have a bunch of static "create...()" members that instantiate an immutable search specification?
>
> As far as the naming is concerned, what's currently called TypeMatch should maybe be called TypeQuery, since match seems to imply a result?

I will rename TypeMatch to TypeQuery and try to see if I can put all mutable parts into a TypeQueryResults,

> Why is FindFirstType a member of TypeMatch? I would have expected a function called FindFirstType to take a TypeMatch search specification as an argument?

It just sets all of the settings needed to do the TypeMatch correctly and then does the query. I avoids a bunch of duplicated code all over the place where call locations were creating a TypeMatch object with some query, then setting the max number of matches to 1 and then doing the type query. So it is a convenience function and allows us to  modify anything needed when finding the first type and all locations that call this won't ever need to change if we modify the TypeMatch API.

I will try and break things up further and see if I can get rid of set accessors of TypeMatch (soon to be TypeQuery. I want to avoid having too many parameters to a constructor or create function, but it depends on how these queries are used all over. Let me do one more round of cleanup and you can let me know what you think,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900