12:11 a.m.
This is a review of this patch and the subsequent one removing
unnecessary blocks.
CMCAuth.java: Can you explain why this code removal is correct?
CAAdminServlet.java : code should be commented out, rather than removed.
HashEnrollServlet.java : remove the outer conditional as well.
DBSubsystem.java: some important comments are removed, they should not
be removed.
FileAsString.java - does the proposed code removal introduce a resource
leak?
KeyRecoveryAuthority.java: please explain why the proposed code removal
is correct. It certainly looks wrong.
Ade
On Wed, 2011-11-09 at 19:50 -0500, Adam Young wrote:
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
9:33 a.m.
On 11/15/2011 01:11 AM, Ade Lee wrote:
This is a review of this patch and the subsequent one removing
unnecessary blocks.
CMCAuth.java: Can you explain why this code removal is correct?
At this point in the code, cert can only be null. The only code path
that assigns a value to cert has a return after it, and cannot reach
this point. Thus, the code executed when cert != null is unreachable
CAAdminServlet.java : code should be commented out, rather than removed.
Disagree. If this code has never been run, it is unnecessary. Lets
not put dead code into the source tree.
HashEnrollServlet.java : remove the outer conditional as well.
Done
DBSubsystem.java: some important comments are removed, they should
not
be removed.
Done
FileAsString.java - does the proposed code removal introduce a resource
leak?
No. FileReader can throw a file not found exception. But
BufferedReader only throws an IllegalArgumentException, which wouldn't
be caught by that catch block anyway.
KeyRecoveryAuthority.java: please explain why the proposed code removal
is correct. It certainly looks wrong.
I agree that the change looks wrong. I put
it back in, and Eclipse did
not tag it as dead code.
Ade
On Wed, 2011-11-09 at 19:50 -0500, Adam Young wrote:
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
7:57 p.m.
On 11/16/2011 10:33 AM, Adam Young wrote:
On 11/15/2011 01:11 AM, Ade Lee wrote:
> This is a review of this patch and the subsequent one removing
> unnecessary blocks.
>
> CMCAuth.java: Can you explain why this code removal is correct?
At this point in the code, cert can only be null. The only code path
that assigns a value to cert has a return after it, and cannot reach
this point. Thus, the code executed when cert != null is unreachable
>
> CAAdminServlet.java : code should be commented out, rather than removed.
Disagree. If this code has never been run, it is unnecessary. Lets
not put dead code into the source tree.
>
> HashEnrollServlet.java : remove the outer conditional as well.
Done
> DBSubsystem.java: some important comments are removed, they should not
> be removed.
Done
>
> FileAsString.java - does the proposed code removal introduce a resource
> leak?
No. FileReader can throw a file not found exception. But
BufferedReader only throws an IllegalArgumentException, which
wouldn't be caught by that catch block anyway.
>
> KeyRecoveryAuthority.java: please explain why the proposed code removal
> is correct. It certainly looks wrong.
I agree that the change looks wrong. I put it back in, and Eclipse
did not tag it as dead code.
>
> Ade
>
> On Wed, 2011-11-09 at 19:50 -0500, Adam Young wrote:
>> _______________________________________________
>> Pki-devel mailing list
>> Pki-devel(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
>
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
Is that an ACK?
8:49 a.m.
On Sun, 2011-11-20 at 20:57 -0500, Adam Young wrote:
On 11/16/2011 10:33 AM, Adam Young wrote:
> On 11/15/2011 01:11 AM, Ade Lee wrote:
> > This is a review of this patch and the subsequent one removing
> > unnecessary blocks.
> >
> > CMCAuth.java: Can you explain why this code removal is correct?
>
> At this point in the code, cert can only be null. The only code
> path that assigns a value to cert has a return after it, and cannot
> reach this point. Thus, the code executed when cert != null is
> unreachable
>
> >
> > CAAdminServlet.java : code should be commented out, rather than
> > removed.
>
> Disagree. If this code has never been run, it is unnecessary.
> Lets not put dead code into the source tree.
> >
> > HashEnrollServlet.java : remove the outer conditional as well.
> Done
> > DBSubsystem.java: some important comments are removed, they should
> > not
> > be removed.
>
> Done
> >
> > FileAsString.java - does the proposed code removal introduce a
> > resource
> > leak?
>
> No. FileReader can throw a file not found exception. But
> BufferedReader only throws an IllegalArgumentException, which
> wouldn't be caught by that catch block anyway.
>
> >
> > KeyRecoveryAuthority.java: please explain why the proposed code
> > removal
> > is correct. It certainly looks wrong.
> I agree that the change looks wrong. I put it back in, and Eclipse
> did not tag it as dead code.
> >
> > Ade
> >
> > On Wed, 2011-11-09 at 19:50 -0500, Adam Young wrote:
> > > _______________________________________________
> > > Pki-devel mailing list
> > > Pki-devel(a)redhat.com
> > > https://www.redhat.com/mailman/listinfo/pki-devel
> >
>
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
Is that an ACK?
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
ACK
12:38 p.m.
On 11/21/2011 09:49 AM, Ade Lee wrote:
On Sun, 2011-11-20 at 20:57 -0500, Adam Young wrote:
> On 11/16/2011 10:33 AM, Adam Young wrote:
>> On 11/15/2011 01:11 AM, Ade Lee wrote:
>>> This is a review of this patch and the subsequent one removing
>>> unnecessary blocks.
>>>
>>> CMCAuth.java: Can you explain why this code removal is correct?
>> At this point in the code, cert can only be null. The only code
>> path that assigns a value to cert has a return after it, and cannot
>> reach this point. Thus, the code executed when cert != null is
>> unreachable
>>
>>> CAAdminServlet.java : code should be commented out, rather than
>>> removed.
>> Disagree. If this code has never been run, it is unnecessary.
>> Lets not put dead code into the source tree.
>>> HashEnrollServlet.java : remove the outer conditional as well.
>> Done
>>> DBSubsystem.java: some important comments are removed, they should
>>> not
>>> be removed.
>> Done
>>> FileAsString.java - does the proposed code removal introduce a
>>> resource
>>> leak?
>> No. FileReader can throw a file not found exception. But
>> BufferedReader only throws an IllegalArgumentException, which
>> wouldn't be caught by that catch block anyway.
>>
>>> KeyRecoveryAuthority.java: please explain why the proposed code
>>> removal
>>> is correct. It certainly looks wrong.
>> I agree that the change looks wrong. I put it back in, and Eclipse
>> did not tag it as dead code.
>>> Ade
>>>
>>> On Wed, 2011-11-09 at 19:50 -0500, Adam Young wrote:
>>>> _______________________________________________
>>>> Pki-devel mailing list
>>>> Pki-devel(a)redhat.com
>>>> https://www.redhat.com/mailman/listinfo/pki-devel
>>
>>
>> _______________________________________________
>> Pki-devel mailing list
>> Pki-devel(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
> Is that an ACK?
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
ACK
Commited to trunk. To to the PKISIlent restructuring, I had to move
the ArgParser code by hand, but the change matches what was done in the
patch.
4781
days inactive
4792
days old
5 comments
2 participants
participants (2)
-
Adam Young -
Ade Lee