Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-11-01 Thread Sean Mullan
On 11/1/18 4:19 AM, Weijun Wang wrote: On Nov 1, 2018, at 5:25 AM, Nico Williams wrote: Otherwise looks fine. You will need to add a noreg label if you can't write a test. Yes. Not sure what that means. https://openjdk.java.net/guide/changePlanning.html#noreg. I'll add a noreg-hard d

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-11-01 Thread Weijun Wang
> On Nov 1, 2018, at 5:25 AM, Nico Williams wrote: > >>> Otherwise looks fine. You will need to add a noreg label if you can't write >>> a test. >> >> Yes. > > Not sure what that means. https://openjdk.java.net/guide/changePlanning.html#noreg. I'll add a noreg-hard due to complex setup. *

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-10-31 Thread Sean Mullan
On 10/31/18 5:25 PM, Nico Williams wrote: I think you should put braces around the conditional statements on lines 332, 357, & 359. It would read better and avoid accidental bugs. Is that part of a published Java style? (Personally, I dislike braces for single statement blocks. But we'll follo

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-10-31 Thread Nico Williams
On Wed, Oct 31, 2018 at 05:22:27PM +0800, Weijun Wang wrote: > > On Oct 30, 2018, at 10:28 PM, Sean Mullan wrote: > > > > I think you should put braces around the conditional statements on lines > > 332, 357, & 359. It would read better and avoid accidental bugs. Is that part of a published Java

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-10-31 Thread Weijun Wang
Copying Nico the original author. > On Oct 30, 2018, at 10:28 PM, Sean Mullan wrote: > > I think you should put braces around the conditional statements on lines 332, > 357, & 359. It would read better and avoid accidental bugs. > > Where does delegatedCred get used? It seems to be never set.

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-10-30 Thread Sean Mullan
I think you should put braces around the conditional statements on lines 332, 357, & 359. It would read better and avoid accidental bugs. Where does delegatedCred get used? It seems to be never set. Otherwise looks fine. You will need to add a noreg label if you can't write a test. --Sean O

RFR 8212217: JGSS: Don't dispose() of creds too eagerly

2018-10-15 Thread Weijun Wang
Please take a review at http://cr.openjdk.java.net/~weijun/8212217/webrev.00/ This bug is reported and fixed by Nico Williams . I'll think if a regression test can be added. Thanks Max