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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor 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);
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98653