On 3 apr 2014, at 14:47, Jaroslav Bachorik <[email protected]> wrote:
> On 3.4.2014 14:33, Staffan Larsen wrote: >> >> On 3 apr 2014, at 14:20, Jaroslav Bachorik <[email protected]> >> wrote: >> >>> On 3.4.2014 14:08, Dmitry Samersoff wrote: >>>> Jaroslav, >>>> >>>> Command line processing is still not clean, e.g. you check for -help in >>>> three different places. >>>> >>>> If you go to refactor this code rather than provide a minimal fix, >>>> it might be better to create a separate inner class that do all command >>>> line processing and provide couple of flags like should_sa(), >>>> should_help(), should_attach() etc. >>> >>> Yes, I know :( >>> >>> My first approach was exactly like that. But I was rewriting the command >>> line parser from scratch for at least the 3rd time for "tools" alone :( And >>> the change was going to be rather big - more of a complete rewrite than a >>> fix. >> >> I just re-wrote that command line parser. Obviously with mixed results… :( >> I think the real problem here is that this command line is not specified in >> a way that is easily parseable, and we don’t want to change how it works. >> >> Command line parsing is one of the really hard areas in computer science. > > There are some rather flexible frameworks allowing nice command line parsing > in java. But they are all 3rd party utilities. > >> >> >> You have added two methods that do not look like they are being called: >> useSA() and getArgs(). > > These two methods are there only to make the code testable without resorting > to reflection and setAccessible(). Not sure what is the lesser evil … My bad - I missed the test. > >> >> Otherwise the changes look good. >> >> If you haven’t already, make sure sun/tools/jinfo/Basic.sh passes on all >> platforms and add cases to it if that is appropriate. (Note that it >> currently fails on solaris JDK-8038296). > Basic.sh should not be affected by this change at all. But I will make sure > the tests pass on all platforms before pushing. Thanks. Reviewed. /Staffan > > -JB- > >> >> Thanks, >> /Staffan
