Hi Ralf, On Fri, Nov 8, 2019 at 4:45 PM Schmelter, Ralf <ralf.schmel...@sap.com> wrote:
> Hi Thomas, > > thanks for the review. > > > > Can you please add a comment about the new parameter? E.g. "optional > outputStream to which progress- and error messages will be written". > > Will do. > > > > - in HeapDumper::dump(): while you are on it could you please initialize > my_path to NULL at function start? > > Do you mean the HeapDumper::dump_heap() method? That seems unrelated to my > change. And I would prefer to get a warning about a potentially > uninitialized use. > > Yes its unrelated but since you are in the area... But okay, I leave it up to you. > > > You can actually get rid of HeapDumper::error_as_C_string() altogher now. > > I thought about it, but the message is more than just the error text (you > would get the "Dumping heap to <file> .." part) and is multiple lines. This > seems not fitting for an exception message. > > Okay. > > > Did you test that no jtreg tests fall over the changed output? e.g. > test/jdk/sun/tools/jmap/BasicJMapTest.java, or whatever is under > hotspot/jtreg/serviceability/jcmd > > I've checked them. They work because they just check that "Heap dump file > created" is contained in the output, which is still the case or don't check > the output at all (the dcmd tests). > > Okay. > Best regards, > Ralf > Okay. If you only add the comment I do not need to see a new webrev. Cheers, Thomas
