[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

[Jun Zhang via Phabricator via cfe-commits]

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