[
https://issues.apache.org/jira/browse/LUCENE-7796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15984575#comment-15984575
]
Dawid Weiss edited comment on LUCENE-7796 at 4/26/17 10:59 AM:
---------------------------------------------------------------
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places where rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+ throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think, especially [~hossman_luc...@fucit.org] since you
provided an alternative view on how this issue could be solved (which I don't
disagree with -- I just think this patch results in better code for now and the
future).
was (Author: dweiss):
TL;DR; I looked at all occurrences of rethrow (ioutils) and I really can't find
any scenario in which accepting null to this method would be clearer or more
convenient, so I decided to follow my original suggestion and not make it
accept null at all.
Here's my motivation. Look at the patch. In most places rethrow was used
without a null check it was "guarded" by some awkward comment about potentially
null argument pass-through:
{code}
if (success) {
- // Does nothing if firstExc is null:
- IOUtils.reThrow(firstExc);
+ if (firstExc != null) {
+ throw IOUtils.rethrowAlways(firstExc);
+ }
}
{code}
This was and is weird to me. The code self-reads if it has an {{if (t != null)
throw rethrow(t);}}, plain and simple, no additional comment required.
I also changed (private) code in a few places where it's I think now easier to
grasp without knowing the semantics of underlying check methods:
{code}
} catch (Throwable t) {
- verifyChecksum(t, source.writer);
+ throw verifyChecksum(t, source.writer);
}
{code}
This reads "always rethrows something". No further comments are required and no
code can follow (javac will complain).
This patch removes the (public) method on IOUtils so that people who are used
to that method would have to find the new one and learn about the different
semantics. I can revert it to be named identical as before if somebody really
insists.
This is for master. For backporting to 6x, we could deprecate the old method
and make it still functional.
Let me know what you think.
> Make reThrow idiom declare RuntimeException return type so callers may use it
> in a way that compiler knows subsequent code is unreachable
> -----------------------------------------------------------------------------------------------------------------------------------------
>
> Key: LUCENE-7796
> URL: https://issues.apache.org/jira/browse/LUCENE-7796
> Project: Lucene - Core
> Issue Type: Improvement
> Reporter: Dawid Weiss
> Assignee: Dawid Weiss
> Priority: Trivial
> Fix For: 6.x, master (7.0)
>
> Attachments: LUCENE-7796.patch
>
>
> A spinoff from LUCENE-7792: reThrow can be declared to return an unchecked
> exception so that callers can choose to use {{throw reThrow(...)}} as an
> idiom to let the compiler know any subsequent code will be unreachable.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
