On Thu, 12 Jan 2023 21:28:12 GMT, Archie L. Cobbs <d...@openjdk.org> wrote:
>> My point is about who puts ThisRef in the set to begin with. It seems to me
>> that ThisRef is put there at the start of a method analysis. After which,
>> there's several code points where we say "if there's a direct ThisRef in the
>> set, do this, otherwise, if there's an indirect ThisRef in the set, do
>> that". But the ThisRef (whether indirect or direct) seems set once and for
>> all (when we start the analysis, and then inside visitApply).
>>
>> There is also this bit in `visitReference`:
>>
>>
>> case SUPER:
>> if (this.refs.contains(ThisRef.direct()))
>> receiverRefs.add(ThisRef.direct());
>> if (this.refs.contains(ThisRef.indirect()))
>> receiverRefs.add(ThisRef.indirect());
>> break;
>>
>>
>> But this doesn't change what I'm saying - there seems to be a general
>> property when we execute this analysis which says whether the current
>> execution context has a direct `this` or not. This seems to me better
>> captured with a boolean, which is then fed back to all the other various
>> downstream ref creation.
>>
>> The main observation, at least for me, is that the code unifies everything
>> under refs, when in reality there are different aspects:
>>
>> * the set of variables that can point to this, whether directly or
>> indirectly (this is truly a set)
>> * whether the current context has a direct or indirect this (this seems more
>> a flag to me)
>> * whether the expression on top of stack is direct/indirect this reference
>> or not (again, 3 possible values here) - but granted, there `depth` here to
>> take into account, so you probably end up with a set (given that we don't
>> want to model a scope with its own set)
>>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> I get what you're saying - it seems silly to model what is essentially a
> fixed, boolean property with the membership of a singleton in a set field,
> rather than with a simple boolean field.
>
> There is a conceptual trade-off though... a lot of the code relates to
> converting `Ref`'s from one type to another. For example, as pointed out
> above, a method invocation might convert a `ExprRef` to a `ThisRef`, then to
> a `ReturnRef`'s, etc. Having these things all be considered part of the same
> family helps conceptually. The fact that a direct `ThisRef` is a singleton is
> just a coincidence in this way of looking at things.
>
> The benefit is the simplicity of being able to think of the data model as
> "just a set of references".
>
> For example, methods like `RefSet.replaceExprs()` become less elegant (or
> basically impossible) if there have to be special cases for each type of
> reference, whereas currently we can do clean stuff like this:
>
>
> @Override
> public void visitReturn(JCReturn tree) {
> scan(tree.expr);
> refs.replaceExprs(depth, ReturnRef::new);
> }
>
>
> But I'm also realizing now that several places can be cleaned up by taking
> this event further. E.g., we should replace this code:
>
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
>
> with something like this:
>
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
>
>
> I will go through and refactor to do that and clean things up a little more.
>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> Probably my fault for not providing better documentation of the overall "set
> of references" conceptual approach. FWIW I added a little bit more in
> f83a9cf0.
> ```java
> ```java
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
> ```
>
>
>
>
>
>
>
>
>
>
>
> with something like this:
> ```java
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> ```
> ```
This sounds like a good idea, that idiom is quite widespread, so it should help
avoiding repetition.
-------------
PR: https://git.openjdk.org/jdk/pull/11874
