[Lldb-commits] [PATCH] D98653: [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

(Just pushing this back into Dave's review queue)



================
Comment at: lldb/include/lldb/Symbol/CompilerDeclContext.h:77
   ///     in a struct, union or class.
-  bool IsClassMethod(lldb::LanguageType *language_ptr,
-                     bool *is_instance_method_ptr,
-                     ConstString *language_object_name_ptr);
+  bool IsClassMethod(ConstString *instance_var_name_ptr = nullptr,
+                     bool *instance_is_pointer_ptr = nullptr);
----------------
jingham wrote:
> kastiglione wrote:
> > teemperor wrote:
> > > kastiglione wrote:
> > > > shafik wrote:
> > > > > If we are going to refactor this, this change does not feel very C++y passing around pointers. I know we want a way to call this w/o any arguments but perhaps we can write an overload for that case?
> > > > > 
> > > > > Does `instance_var_name_ptr` need to be a string? Maybe we can encode it using an enum, we don't have a lot of cases `this`, `self`, maybe even not a pointer as well and get ride of `instance_is_pointer_ptr`.
> > > > Something like?
> > > > 
> > > > ```
> > > > enum InstanceVariable {
> > > >    ThisPointer,
> > > >    SelfPointer,
> > > >    Self,
> > > > };
> > > > ```
> > > We could also make this function that is something like `llvm::Optional<SelfRef> GetCurrentObjectRef` and `SelfRef` is just ConstString + enum if it's a pointer/ref/whatever.
> > > 
> > > FWIW, encoding the string inside an enum doesn't seem to fit with the idea that the TypeSystem interface just needs to be implemented (but not extended) when adding a new language plugin (if the language uses a different name like `$this` or `Self` then the enum needs to be expanded). Also not sure what use this has to the caller (I don't see how the callers do anything else with this enum then translating it to the actual string and checking if it's a pointer, both are more complicated with an enum).
> > I like this approach. Before I make the change, some questions/thoughts.
> > 
> > I'm thinking the second field will be a bool, ex: `is_pointer`. The reason for bool and not enum is that I don't know if it's worth the complexity of trying to distinguish between reference and value. In Swift, the `self` variable could be reference (`class`) or value (`struct`, `enum`…).
> > 
> > Instead of `SelfRef` I'm thinking `{This,Self,Instance}Variable`, since it's info about the variable (name, pointer-ness).
> > 
> > Do we need to return a `ConstString`, or can it be `const char *` and let the caller do what it wants. It seems it will always be a string literal, and `const char *` is a lower common denominator. I guess I'm ultimately unclear on when, if ever, to not use `ConstString`?
> ConstString's main purpose is to hold strings we're likely to compare against a lot.  For instance, if you take a symbol name and you are going to look it up everywhere, it's appropriate for that to be a ConstString since we're going to turn it into that anyway to do the searches.
> 
> Since a caller is likely to turn around and look up "this" having gotten that name back, a ConstString seems an okay choice.  Another way to do this would be to make function statics with ConstStrings for "this" and "self".  When you make a ConstString from a c-string we have to hash it look for it in the string pool.  Copying a ConstString is just copying a pointer.  So if you have just a couple of options, making static ConstStrings makes returning the result cheap.  And since ConstString's are all null-terminated C-strings, ConstString -> cstring is cheap.
FWIW, the available string types you could use would be:

* `llvm::StringRef` -> no ownership and cheap
* `std::string` -> ownership
* `ConstString` -> Can be very cheap or expensive depending on how you use it.
* `const char *` -> Nearly always a bad idea (exceptions are stuff that do C interop).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98653