I am supportive of this change (the risk to impacting order-dependent stateful MH filter code is smaller than the risk of hitting a string concatenation bug). (We erred on the side of this being a bug and not being a spec change given the pseudo-code in the Java doc.)
IIRC this will require a nod of approval from the project lead. Paul. > On Apr 9, 2018, at 2:52 AM, Aleksey Shipilev <sh...@redhat.com> wrote: > > Ping. > > -Aleksey > > On 04/05/2018 05:33 PM, Aleksey Shipilev wrote: >> Hi, >> >> Please review this jdk10 backport. >> >> Original JDK 11 bug: >> https://bugs.openjdk.java.net/browse/JDK-8194554 >> >> Original JDK 11 fix: >> http://hg.openjdk.java.net/jdk/jdk/rev/050352ed64d5 >> >> Please note the discussion in JBS comments about the issue. It seems to >> include the more verbose >> specification text, and Rob says it makes the patch not directly >> backportable. Therefore, requesting >> to backport without the Javadoc change. I just dropped that single line from >> the original changeset: >> >> >> 8194554: filterArguments runs multiple filters in the wrong order >> Reviewed-by: psandoz, jrose >> >> diff -r b09e56145e11 -r ab2dc3096cdb >> src/java.base/share/classes/java/lang/invoke/MethodHandles.java >> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java >> Thu Mar 08 04:23:31 2018 +0000 >> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java >> Wed Jan 17 15:17:50 2018 -0800 >> @@ -3836,11 +3836,12 @@ >> MethodHandle filterArguments(MethodHandle target, int pos, >> MethodHandle... filters) { >> filterArgumentsCheckArity(target, pos, filters); >> MethodHandle adapter = target; >> - int curPos = pos-1; // pre-incremented >> - for (MethodHandle filter : filters) { >> - curPos += 1; >> + // process filters in reverse order so that the invocation of >> + // the resulting adapter will invoke the filters in left-to-right >> order >> + for (int i = filters.length - 1; i >= 0; --i) { >> + MethodHandle filter = filters[i]; >> if (filter == null) continue; // ignore null elements of >> filters >> - adapter = filterArgument(adapter, curPos, filter); >> + adapter = filterArgument(adapter, pos + i, filter); >> } >> return adapter; >> } >> diff -r b09e56145e11 -r ab2dc3096cdb >> test/jdk/java/lang/invoke/FilterArgumentsTest.java >> --- /dev/null Thu Jan 01 00:00:00 1970 +0000 >> +++ b/test/jdk/java/lang/invoke/FilterArgumentsTest.java Wed Jan 17 >> 15:17:50 2018 -0800 >> @@ -0,0 +1,132 @@ >> +/* >> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. >> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >> + * >> + * This code is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 only, as >> + * published by the Free Software Foundation. >> + * >> + * This code is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> + * version 2 for more details (a copy is included in the LICENSE file that >> + * accompanied this code). >> + * >> + * You should have received a copy of the GNU General Public License version >> + * 2 along with this work; if not, write to the Free Software Foundation, >> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. >> + * >> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA >> + * or visit www.oracle.com if you need additional information or have any >> + * questions. >> + */ >> + >> +/* >> + * @test >> + * @bug 8194554 >> + * @run testng/othervm test.java.lang.invoke.FilterArgumentsTest >> + */ >> + >> +package test.java.lang.invoke; >> + >> +import java.lang.invoke.MethodHandle; >> +import java.lang.invoke.MethodHandles; >> +import java.util.ArrayList; >> +import java.util.List; >> + >> +import static java.lang.invoke.MethodHandles.*; >> +import static java.lang.invoke.MethodType.methodType; >> + >> +import org.testng.annotations.*; >> +import static org.testng.Assert.*; >> + >> +public class FilterArgumentsTest { >> + >> + @Test >> + public static void testFilterA_B_C() throws Throwable { >> + FilterTest test = new FilterTest( >> + filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B, >> MH_FILTER_C)); >> + test.run(List.of("A", "B", "C")); >> + } >> + >> + @Test >> + public static void testFilterA_B() throws Throwable { >> + FilterTest test = new FilterTest( >> + filterArguments(MH_TEST, 0, MH_FILTER_A, MH_FILTER_B)); >> + test.run(List.of("A", "B")); >> + } >> + >> + @Test >> + public static void testFilterB_C() throws Throwable { >> + FilterTest test = new FilterTest( >> + filterArguments(MH_TEST, 1, MH_FILTER_B, MH_FILTER_C)); >> + test.run(List.of("B", "C")); >> + } >> + >> + @Test >> + public static void testFilterB() throws Throwable { >> + FilterTest test = new FilterTest(filterArguments(MH_TEST, 1, >> MH_FILTER_B)); >> + test.run(List.of("B")); >> + } >> + >> + @Test >> + public static void testFilterC() throws Throwable { >> + FilterTest test = new FilterTest(filterArguments(MH_TEST, 2, >> MH_FILTER_C)); >> + test.run(List.of("C")); >> + } >> + >> + static class FilterTest { >> + static List<String> filters = new ArrayList<>(); >> + >> + final MethodHandle mh; >> + FilterTest(MethodHandle mh) { >> + this.mh = mh; >> + } >> + >> + void run(List<String> expected) throws Throwable { >> + filters.clear(); >> + assertEquals("x-0-z", (String)mh.invokeExact("x", 0, 'z')); >> + assertEquals(expected, filters); >> + } >> + >> + static String filterA(String s) { >> + filters.add("A"); >> + return s; >> + } >> + >> + static int filterB(int value) { >> + filters.add("B"); >> + return value; >> + } >> + >> + static char filterC(char c) { >> + filters.add("C"); >> + return c; >> + } >> + >> + static String test(String s, int i, char c) { >> + return s + "-" + i + "-" + c; >> + } >> + } >> + >> + static final MethodHandle MH_TEST; >> + static final MethodHandle MH_FILTER_A; >> + static final MethodHandle MH_FILTER_B; >> + static final MethodHandle MH_FILTER_C; >> + static final Lookup LOOKUP = MethodHandles.lookup(); >> + >> + static { >> + try { >> + MH_TEST = LOOKUP.findStatic(FilterTest.class, "test", >> + methodType(String.class, String.class, int.class, >> char.class)); >> + MH_FILTER_A = LOOKUP.findStatic(FilterTest.class, "filterA", >> + methodType(String.class, String.class)); >> + MH_FILTER_B = LOOKUP.findStatic(FilterTest.class, "filterB", >> + methodType(int.class, int.class)); >> + MH_FILTER_C = LOOKUP.findStatic(FilterTest.class, "filterC", >> + methodType(char.class, char.class)); >> + } catch (Exception e) { >> + throw new RuntimeException(e); >> + } >> + } >> +} >> >> >> Thanks, >> -Aleksey >> >
