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

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
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);
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98653