Hi Alan, On Thu, 2019-02-14 at 15:26 +0000, Alan Bateman wrote: > On 13/02/2019 11:16, Severin Gehwolf wrote: > > Hi, > > > > As discussed in the RFR thread for JDK-8214796 it was suggested to > > rename the --strip-debug plugin so that --strip-debug could actually > > perform java debug symbols stripping *and* native debug symbols > > stripping at once. That's what this patch does. It renames --strip- > > debug to --strip-java-debug-symbols and --strip-debug just calls -- > > strip-java-debug-symbols behind the scenes. > > > > Note that --strip-debug-attributes as name would work for me too, but > > given that we are about to introduce --strip-native-debug-symbols, > > having the java strippping plugin with a similar name made sense to me. > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8218913 > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/ > > > I skimmed through the webrev and the approach looks good.
Thanks for the review! > I don't have a strong opinion on the naming but the previous suggestion > to use --strip-java-debug-attributes seems slightly better, if only it > gives a hint that it strips the LVT and LLVT class file attributes. If > the "strip the world" option is --strip-debug-info then it could imply > both --strip-java-debug-attributes and --strip-native-debug-symbols > without needing to unify the terminology. A bit of clarification would be needed here I think: 1) The current option is called '--strip-debug', not '--strip-debug-info'. Did you mean to say to rename --strip-debug to --strip-debug-info too? Perhaps make '--strip-debug' and alias for '--strip-debug-info'? 2) --strip-debug-attributes as a replacement for what --strip-debug does today was discussed. The above patch uses --strip-java-debug-symbols. Now you say --strip-java-debug-attributes. Is that the name I should be using? Currently, my understanding is to go with: --strip-debug => --strip-java-debug-attributes and let --strip-debug just call the --strip-java-debug-attributes plugin. --strip-debug-info would not exist (as it doesn't exist now). Thanks, Severin
