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 ...
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.
-JB-
Thanks,
/Staffan
