Hi all,
ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. Thanks. Best regards, Jie [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning ------------- Commit messages: - 8261938: ASN1Formatter.annotate should not return in the finally block Changes: https://git.openjdk.java.net/jdk/pull/2620/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261938 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620 |
> Hi all,
> > ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Update the copyright year ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2620/files - new: https://git.openjdk.java.net/jdk/pull/2620/files/c0af12b1..b537e060 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620 |
On Thu, 18 Feb 2021 02:46:53 GMT, Jie Fu <[hidden email]> wrote:
>> Hi all, >> >> ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. >> But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. >> >> Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. >> >> Thanks. >> Best regards, >> Jie >> >> [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 >> [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 >> [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning > > Jie Fu has updated the pull request incrementally with one additional commit since the last revision: > > Update the copyright year An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer. Otherwise it is discarded. ------------- Changes requested by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2620 |
On Thu, 18 Feb 2021 14:53:17 GMT, Roger Riggs <[hidden email]> wrote:
>> Jie Fu has updated the pull request incrementally with one additional commit since the last revision: >> >> Update the copyright year > > An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer. Otherwise it is discarded. Thanks @RogerRiggs for your review. Just want to make sure: 1. AFAIK, for a method, it seems impossible to return a value and throw an exception at the same time. Did you mean we just need to return a string with the IOException to be catched? 2. Does it make sense to return a string when an IOException happens? Thanks. ------------- PR: https://git.openjdk.java.net/jdk/pull/2620 |
On Thu, 18 Feb 2021 23:41:08 GMT, Jie Fu <[hidden email]> wrote:
>> An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer. Otherwise it is discarded. > > Thanks @RogerRiggs for your review. > > Just want to make sure: > > 1. AFAIK, for a method, it seems impossible to return a value and throw an exception at the same time. > Did you mean we just need to return a string with the IOException to be catched? > > 2. Does it make sense to return a string when an IOException happens? > > Thanks. The formattters are a test component used both standalone and in the context of the HexPrinter test utilities. In typical use, the stream is a wrapped byte array, so there are no exceptions other than EOF. The choice of DataInputStream was chosen for the convenience of the methods to read different types and (declared) exceptions are an unwelcome artifact. Formatters are designed to be nested, where one formatter can call another and the valuable output is the formatted string that has been accumulated from the beginning of the stream. If an exception was percolated up and the formatted output discarded it would defeat the purpose. If an exception was thrown, it would still return useful information about the stream to that point. The documentation could be improved to be clear on that point. ------------- PR: https://git.openjdk.java.net/jdk/pull/2620 |
In reply to this post by Jie Fu-2
> Hi all,
> > ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Catch exception and return the accumulated string ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2620/files - new: https://git.openjdk.java.net/jdk/pull/2620/files/b537e060..a1f29f33 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=01-02 Stats: 16 lines in 1 file changed: 13 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620 |
In reply to this post by Roger Riggs-2
On Fri, 19 Feb 2021 01:56:19 GMT, Roger Riggs <[hidden email]> wrote:
> The formattters are a test component used both standalone and in the context of the HexPrinter test utilities. > In typical use, the stream is a wrapped byte array, so there are no exceptions other than EOF. > The choice of DataInputStream was chosen for the convenience of the methods to read different types > and (declared) exceptions are an unwelcome artifact. > > Formatters are designed to be nested, where one formatter can call another and the valuable output > is the formatted string that has been accumulated from the beginning of the stream. > If an exception was percolated up and the formatted output discarded it would defeat the purpose. > > If an exception was thrown, it would still return useful information about the stream to that point. > The documentation could be improved to be clear on that point. Got it. Thanks for your clarification. Updated. Thanks. ------------- PR: https://git.openjdk.java.net/jdk/pull/2620 |
In reply to this post by Jie Fu-2
On Fri, 19 Feb 2021 03:36:00 GMT, Jie Fu <[hidden email]> wrote:
>> Hi all, >> >> ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. >> But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. >> >> Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. >> >> Thanks. >> Best regards, >> Jie >> >> [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 >> [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 >> [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning > > Jie Fu has updated the pull request incrementally with one additional commit since the last revision: > > Catch exception and return the accumulated string Catching and ignoring the exception has the same behavior as handling it with finally. ------------- Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2620 |
In reply to this post by Jie Fu-2
On Thu, 18 Feb 2021 02:28:01 GMT, Jie Fu <[hidden email]> wrote:
> Hi all, > > ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning This pull request has now been integrated. Changeset: b10376ba Author: Jie Fu <[hidden email]> URL: https://git.openjdk.java.net/jdk/commit/b10376ba Stats: 15 lines in 1 file changed: 10 ins; 1 del; 4 mod 8261938: ASN1Formatter.annotate should not return in the finally block Reviewed-by: rriggs ------------- PR: https://git.openjdk.java.net/jdk/pull/2620 |
In reply to this post by Roger Riggs-2
On Fri, 19 Feb 2021 15:15:05 GMT, Roger Riggs <[hidden email]> wrote:
>> Jie Fu has updated the pull request incrementally with one additional commit since the last revision: >> >> Catch exception and return the accumulated string > > Catching and ignoring the exception has the same behavior as handling it with finally. Thanks @RogerRiggs . ------------- PR: https://git.openjdk.java.net/jdk/pull/2620 |
Free forum by Nabble | Edit this page |