[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 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?

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?

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



================
Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+      llvm::SmallVectorImpl<lldb_private::CompilerContext> &context) const;
+
----------------
Why can't this be a return value? The context objects are tiny.


================
Comment at: lldb/include/lldb/Symbol/CompilerDeclContext.h:66
+  void GetCompilerContext(
+      llvm::SmallVectorImpl<lldb_private::CompilerContext> &context) const;
+
----------------
same here


================
Comment at: lldb/include/lldb/Symbol/Type.h:185
+  /// context, false if this is normal type lookup.
+  TypeMatch(const llvm::ArrayRef<lldb_private::CompilerContext> &context,
+            bool module_search);
----------------
An ArrayRef is meant to be passed by value.


================
Comment at: lldb/include/lldb/Symbol/Type.h:197
+
+  /// Check of a CompilerContext array from matching type from a symbol file
+  /// matches the \a m_context.
----------------
if?


================
Comment at: lldb/include/lldb/Symbol/Type.h:241
+  /// Add a language family to the list of languages that should produce a match.
+  void AddLanguage(lldb::LanguageType language);
+
----------------
Is this necessary, or could this be rolled into the constructor?


================
Comment at: lldb/include/lldb/Symbol/Type.h:306
+  /// more complete are are used when lookup up types in a clang module's debug
+  /// information.
+  bool m_module_search = false;
----------------
This sentence is missing at least one word. (I also don't quite understand the purpose of this flag)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900