junaire added inline comments.
================
Comment at: clang/include/clang/Interpreter/Value.h:121
+ static T cast(const Value &V) {
+ if (V.isPointerOrObjectType())
+ return (T)(uintptr_t)V.getAs<void *>();
----------------
aaron.ballman wrote:
> v.g.vassilev wrote:
> > aaron.ballman wrote:
> > > Do we have to worry about member function pointers where a single pointer
> > > value may not be sufficient?
> > I am not sure if I understand your comment correctly. We only store objects
> > and not function/member pointers (perhaps in some pathological way one
> > could do that but I'd say it is not supported). Here we want to know if we
> > stored it as a non-builtin type we need to make the pointer-like casts. My
> > take is that for now we shouldn't worry about member function pointers.
> Given:
> ```
> struct Foo {
> struct Base {
> virtual int bar();
> };
>
> struct Foo : Base {
> int bar() { return 12; }
> };
>
> int (Foo::*bar_ptr)() = &Foo::bar;
> ```
> The object `bar_ptr` requires two pointers of space to represent:
> https://godbolt.org/z/o81sbjKM6, so I'm wondering whether `Value` can
> represent `bar_ptr`
I added a test case for this.
================
Comment at: clang/include/clang/Interpreter/Value.h:143
+ /// Values referencing an object are treated as pointers to the object.
+ template <typename T> T castAs() const { return CastFwd<T>::cast(*this); }
+
----------------
aaron.ballman wrote:
> v.g.vassilev wrote:
> > junaire wrote:
> > > aaron.ballman wrote:
> > > > This doesn't match the usual pattern used in Clang where the `get`
> > > > variant returns `nullptr` and the `cast` variant asserts if the cast
> > > > would be invalid. Should we go with a similar approach?
> > > These APIs are adopted in Cling and Cling will remove the old
> > > implementation and use the upstream version of the value printing feature
> > > at some point in the future. So @v.g.vassilev hope we can keep these APIs
> > > close to Cling's variants as much as we can. I don't have a strong
> > > opinion here, what's your take here? @v.g.vassilev
> > I probably see the confusion. The `getAs<T>` operation is only relevant for
> > pointer types. For example we stored object of type `UserClass` as a
> > pointer. For instance, I don't see how `getAs<float>` can return a
> > `nullptr`. We can probably inline the implementation of `getAs` for
> > non-pointer types into `castAs` and leave `getAs` unimplemented relying
> > only on the explicit specialization bellow for pointers.
> >
> > The `castAs` operation means take a valid bit representation and transforms
> > it into the requested one. I don't think it can or should be ever invalid.
> > Here we are reshuffling bits.
> >
> > These changes are very subtle and if we decide to make changes we should
> > open a pull request against the ROOT project to make sure we are not
> > breaking some important case. The reference implementation is here:
> > https://github.com/root-project/root/blob/master/interpreter/cling/include/cling/Interpreter/Value.h
> I think `castAs` can be invalid -- if you have the bits for a float and you
> try to cast it as a pointer, that's not going to lead to good things, right?
>
> My point is largely that the names `getAs` and `castAs` have meaning within
> Clang already and this seems to assign slightly different meaning to them,
> which might trip folks up. It might be worth considering renaming them to
> something like `bitCast` and `pointerCast` (or something else, I'm not tied
> to these names)?
We want to rename:
getAs --> as
castAs --> convertTo
Does these look good to you?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141215/new/
https://reviews.llvm.org/D141215
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
