Quite a large number of files, indeed. Lgtm. Paul
On 2/20/19, 9:36 AM, "serviceability-dev on behalf of Chris Plummer" <[email protected] on behalf of [email protected]> wrote: I think because of the number of files changed it probably should not be considered trivial. For anyone interested in doing a review, just look at the patch file. It's pretty easy to skim through. Chris On 2/20/19 5:20 AM, Gary Adams wrote: > Can I have a second reviewer or a confirmation that this is a trivial > change > only needing one reviewer? > > On 2/19/19, 6:15 PM, Chris Plummer wrote: >> Looks good. >> >> Chris >> >> On 2/19/19 1:52 PM, [email protected] wrote: >>> Sorry, my bad, used the wrong list of files when I made the webrev. >>> Added the location, interrupt and setvalue debuggee references. >>> >>> Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/ >>> >>> On 2/19/19 3:01 PM, Chris Plummer wrote: >>>> Hi Gary, >>>> >>>> On 2/19/19 11:38 AM, Gary Adams wrote: >>>>> On my first pass I went after cleaning up the issuspend002a messages, >>>>> because I'm investigating pulling it off the ProblemList. >>>>> >>>>> I added the setvalue003a misleading log messages in this updated >>>>> webrev. >>>> 3 of the ones I pointed out below that were not setvalue003a. >>>>> I think the raw "debugee launched" messages are in context of >>>>> other messages. >>>>> I'd prefer to leave that level of clean to a future effort. >>>> Ok. >>>>> >>>>> Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/ >>>> >>>> There's something wrong with your webrev. Look at the end of the >>>> index page. Those files showing no diffs are all duplicates, and >>>> also show up twice in the patch file. I also don't see the >>>> setvalue003a changes in it. >>>> >>>> Chris >>>> >>>>> >>>>> On 2/19/19, 1:59 PM, Chris Plummer wrote: >>>>>> Hi Gary, >>>>>> >>>>>> Changes look good, but there are other similar incorrect messages >>>>>> you might also want to address: >>>>>> >>>>>> ./StackFrame/thisObject/thisobject002.java:158: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./StackFrame/thisObject/thisobject001.java:159: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./StackFrame/thread/thread001.java:156: log2("location001a >>>>>> debuggee launched"); >>>>>> ./StackFrame/visibleVariables/visiblevariables001.java:156: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./StackFrame/visibleVariables/visiblevariables002.java:153: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./ThreadReference/interrupt/interrupt001.java:156: >>>>>> log2("interrupt002a debuggee launched"); >>>>>> ./LocalVariable/isVisible/isvisible001.java:162: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./ClassType/invokeMethod/invokemethod001.java:162: >>>>>> log2("location001a debuggee launched"); >>>>>> ./Value/type/type002/type002.java:199: log2("setvalue003a >>>>>> debuggee launched"); >>>>>> ./VoidValue/equals/equals001/equals001.java:207: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> ./VoidValue/hashCode/hashcode001/hashcode001.java:207: >>>>>> log2("setvalue003a debuggee launched"); >>>>>> >>>>>> And I noticed a very large number of tests that only have: >>>>>> >>>>>> log2("debuggee launched"); >>>>>> >>>>>> But there are so many that if you are to fix them, it should be >>>>>> done as a separate CR. >>>>>> >>>>>> Chris >>>>>> >>>>>> On 2/19/19 10:19 AM, Gary Adams wrote: >>>>>>> A log message should have been parameterized >>>>>>> with the debuggeeName. >>>>>>> >>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8219388 >>>>>>> Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.00/ >>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >> >
