[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#3925775 <https://reviews.llvm.org/D137900#3925775>, @aprantl wrote:

> Thanks for looking into this. I'm not sure I like how `TypeMatch` is both input and output of the type search. What do you think about making an immutable "TypeQuery" object that describes the search characteristics and have the API return a TypeMap?

The main issue is TypeMap has outlawed copy constructors and assignment operators for some reason. And the reason to contain all of the results in the object itself is because we often call a type search on a specific module first and then fall back to other modules. Having the TypeMap be part of the TypeMatch means the TypeMatch object can make decisions on if it is done or not (found the max number of matches, etc) and we only need to pass one object into each call and avoid copying and merging TypeMap objects which will just add inefficiency. So think of an API where we have:

  TypeMap Module::FindTypes(TypeMatch &match);

If we want to search a module list for a type using this API, we will need to be able to pass the existing TypeMap result into each call to the above API.

  TypeMap ModuleList::FindTypes(TypeMatch &match) {
    TypeMap result;
    for (auto module: m_modules) {
      TypeMap module_result = module.FindTypes(match);
      result.InsertUnique(module_result); // Copying things from the module_result to result will be a bit inefficient here
      // And now to check if the type match is done we need to pass this TypeMap into the match object
      if (match.Done(result))
        return;
    }
  }

Right now the TypeMatch object has all of the state inside of it and it doesn't allow for coding mistakes, like if above someone instead of running "match.Done(result)" accidentally did "match.Done(module_result)" (using the wrong TypeMap to check if the matching is done. This is why I ended up with a single object that doesn't allow for any mistakes when using the APIs.

Let me know if you feel strongly about modifying this.

> Apart from making it easier to reason about it would, e.g., allow caching the result of a type lookup. I'm not sure what to du about the "visited" map in that case. Maybe it could be a `mutable` member of the query object.



> Ideally the TypeQuery object would have only constructors and getters and no methods that change its state.

I will try and get rid of the SetMatchContext functions and use only constructors and see how it looks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900