[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

It seemed to me like Greg thought you were changing the behavior of lookups, which this patch doesn't do, it just makes it more efficient.  I don't know if that alters his objections or not.

The Module and higher layer of FindTypes calls are awkward.  For instance Module::FindTypes takes an "exact_match" parameter, but it doesn't actually mean "exact match".  If exact_match is passed in as true, then it does mean exactly match the name.  But if it is false it means "check if this name is fully qualified and if so make it an exact match otherwise do a partial match".

The header comment for the function is wrong, too.  It says:

  /// @param[in] exact_match
  ///     If \b true, \a type_name is fully qualified and must match
  ///     exactly. If \b false, \a type_name is a partially qualified
  ///     name where the leading namespaces or classes can be
  ///     omitted to make finding types that a user may type
  ///     easier.

So we should change the variable name (or even just the docs) at this level to better reflect what the parameter actually does.  You still need to turn off the guessing because we don't have a 100% certain way to tell whether a type name is fully qualified, and the alternative of prepending a "::" to force the guessing algorithm's hand is gross (and very C++ specific).  But with that proviso, I don't think passing this as a parameter is any better/worse than having Module:FindTypesExact and Module::FindTypesAutoDetect or some better name that I can't think up right now.

But I don't think you need the third "FindTypesPartial" that forces a partial match is useful.  I don't see a use for "this name really looks fully qualified but I want you to treat it as a partial name".  So expressing this as a bool parameter seems fine to me.

OTOH, I don't think it is desirable to have different versions of the Symfile and lower calls for Exact and not Exact, passing the exact_match parameter through there is fine IMO.  After all, in all these calls exact_match means exactly what it says, you are requiring an exact match.  So you don't gain clarity by having different function names.

BTW, you should remove the O(1) FIXME comment so you don't send some future eager contributor on a wild goose chase to figure out how to do this thing you can't do in DWARF.


https://reviews.llvm.org/D53662