deduplicating lambda methods

[Vicente Romero]

On 03/19/2018 10:41 AM, B. Blaser wrote:
> On 19 March 2018 at 14:10, Vicente Romero <vicente.romero at oracle.com> wrote:
>>
>> On 03/17/2018 10:04 PM, Liam Miller-Cushon wrote:
>>> Hi Vicente,
>>>
>>> On Sat, Mar 17, 2018 at 11:44 AM Vicente Romero <vicente.romero at oracle.com
>>> <mailto:vicente.romero at oracle.com>> wrote:
>>>
>>>      overall looks good, but I think that a set of regression tests
>>>      should be included along with the patch.
>>>
>>>
>>> Definitely, I have some functional tests that I'll work on turning into a
>>> jtreg test.
>>>
>>> Do you have advice about how much test coverage is appropriate for tree
>>> comparison?
>>> Getting to 100% branch coverage would require a lot of test cases. I could
>>> probably
>>> generate some of them programatically. FWIW a certain amount of TreeDiffer
>>> was
>>> generated and not hand-written, so any bugs are probably structural
>>> problems rather
>>> than typos. (That doesn't lessen the need for tests, of course--there's
>>> still the risk of
>>> adding typos when changes are made in the future.)
>>
>> well I assume that covering most common cases with a combo test should be
>> the general guideline here. I don't think we need 100% coverage to validate
>> the approach. I would also add some corner cases, probably in separate
>> tests, those corner cases should include lambdas returning lambdas, lambdas
>> with inner classes in their bodies, etc.
> Just playing a bit with this patch, I note that the following example
> isn't de-duplicated correctly:
>
>      Runnable r = () -> { int i = 0; if (i==0) {} };
>      r = () -> { int i = 0; if (i==0) {} };
>
> because symbols don't seem to be '==' for local variables (is that
> right?).

right each local should have a different symbol

>   I think we should probably consider using the name instead
> for local variables, as next.

working with names could be brittle although it should probably 
discriminate a lot of cases due to copy / paste, but I guess if we could 
use other info like type and some positional information

>
> Bernard

Vicente

>
>
> $ diff -u ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java.r03
> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java
> --- ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java.r03
>     2018-03-19 14:59:49.351288750 +0100
> +++ ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java
>     2018-03-19 15:03:36.416434155 +0100
> @@ -172,6 +172,10 @@
>                   result = index == otherIndex;
>                   return;
>               }
> +            else {
> +                result = symbol.name == otherSymbol.name;
> +                return;
> +            }
>           }
>           result = tree.sym == that.sym;
>       }
>
> $ diff -u ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java.r03
> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java
> --- ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java.r03
>     2018-03-19 15:00:02.743120393 +0100
> +++ ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java
>     2018-03-19 15:01:35.581953251 +0100
> @@ -84,6 +84,10 @@
>                   hash(idx);
>                   return;
>               }
> +            else {
> +                hash(tree.sym.name);
> +                return;
> +            }
>           }
>           hash(sym);
>       }