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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
