Good to me, /Staffan
On 15 apr 2014, at 12:57, Jaroslav Bachorik <[email protected]> wrote: > Hi, > > On 8.4.2014 13:31, Jaroslav Bachorik wrote: >> On 7.4.2014 14:25, Jaroslav Bachorik wrote: >>> On 7.4.2014 14:23, shanliang wrote: >>>> shanliang wrote: >>>>> Jaroslav, >>>>> >>>>> Is it necessary to add "ValidationException"? >> >> Replaced the custom ValidationException with IllegalArgumentException() >> to remove an unnecessary inner class. >> >> http://cr.openjdk.java.net/~jbachorik/8039080/webrev.02 > > Could I get the latest changes reviewed, please? > > Thanks, > > -JB- > >> >> -JB- >> >>>>> >>>>> Could we change the constructor JInfo to: >>>>> private static boolean validateArgs(String[] args); >>>>> the method returns false if the args are illegal, or throw an >>>>> IllegalArgumentException >>>>> >>>>> and declare the variables "args" and "useSA" as static too, >>>> Static variables may have problem if called with multi-thread, but we >>>> still could do: >>>> >>>> private static Map<String[], boolean> validate(String[] args) throws >>>> IllegalArgumentException; >>>> >>>> the return map contains args(String[]) and useSA(boolean). >>> >>> ... or just keep them as instance variables. >>> >>> -JB- >>> >>>> >>>> Or put "USE_SA" as a new arg into the args list, then the method >>>> becomes: >>>> private static String[] validate(String[] args) throws >>>> IllegalArgumentException; >>>> >>>> Shanliang >>>> >>>>> >>>>> Shanliang >>>>> >>>>> Jaroslav Bachorik wrote: >>>>>> Hi, >>>>>> >>>>>> Sorry for the noise but I need to get the fix re-reviewed. >>>>>> Due to the way jtreg cooperates with TestNG when runnning in agentvm >>>>>> I can not use package private methods or constructor or fields. >>>>>> >>>>>> The updated patch - >>>>>> http://cr.openjdk.java.net/~jbachorik/8039080/webrev.01 - makes the >>>>>> JInfo constructor a private one and removes the package private >>>>>> getters. The test is using reflection to create new instances of >>>>>> JInfo and to assert its state. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> -JB- >>>>> >>>> >>> >> >
