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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to