deduplicating lambda methods
vicente.romero at oracle.com
Mon Mar 19 13:10:02 UTC 2018
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.
> at first glance it seemed to me that the hash function would
> return the same value for:
> (int i) -> i / 5; and (int i) -> 5 / i; I haven't test it but this
> was my impression.
> The hash function currently traverses all nodes and hashes their tags,
> so for those two
> examples it would see something like `[DIV, IDENT, LITERAL]` and
> `[DIV, LITERAL, IDENT]`,
> which would not result in the same value.
yep I got that but I didn't test it manually, thanks for checking it
> This does mean that `x + 42` and `y + 43` both hashed to the same
> value. I updated the
> hash function to include literal values and symbols (with the same
> handling of method
> parameters as the diff). With that change I am no longer seeing hash
> collisions that result in
> unnecessary diffing. There are still cases where two lambda bodies
> hash to the same
> value but have different target types, but that case doesn't require a
> tree diff (the check
> that the target types are the same happens first).
> The updates to hashing are here:
More information about the amber-dev