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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
