Details

Description

The recent modifications regarding checked exceptions have eliminated the need for several try/catch blocks. This commit removes the blocks that no longer serve a purpose.

Patch: clj949-patch-v2.diff

The attachment clj949-patch-v2-ignore-whitespace.txt is not the intended patch. It was created to be slightly easier to read and review that clj949-patch-v2.txt. It was created by ignoring white space changes using 'git diff -w ...'

Activity

Patch 0001-let-undeclared-exceptions-continue-unchecked.patch applies cleanly to latest master as of March 9, 2012, and build and test without errors or warnings. One author, Brian Taylor, has signed CA.

Andy Fingerhut
added a comment - 09/Mar/12 9:06 AM Patch 0001-let-undeclared-exceptions-continue-unchecked.patch applies cleanly to latest master as of March 9, 2012, and build and test without errors or warnings. One author, Brian Taylor, has signed CA.

Marked incomplete. Code cleanup is good, but I don't think this patch includes all the cases where we can remove try/catch. For example, I found these two by searching for "sneakyThrow" and I expect there are more. If we can, I'd like to get all the possible changes in one patch. Thanks, -S

Stuart Sierra
added a comment - 02/Aug/13 1:45 PM Marked incomplete. Code cleanup is good, but I don't think this patch includes all the cases where we can remove try/catch. For example, I found these two by searching for "sneakyThrow" and I expect there are more. If we can, I'd like to get all the possible changes in one patch. Thanks, -S

Patch clj949-patch-v2.txt dated Aug 30 2013 contains 2 commits. The first commit is the same as Brian Taylor's 0001-let-undeclared-exceptions-continue-unchecked.patch. The second commit removes all remaining occurrences of "throw sneakyThrow(e)" that can be removed without causing Java compilation errors.

clj949-patch-v2-ignore-whitespace.txt is nearly identical, and is included only for ease of review. It was produced with no lines containing only whitespace changes, using the command below, since there are many lines that were in try blocks that were only changed by removing a leading tab character:

Andy Fingerhut
added a comment - 30/Aug/13 8:59 PM Patch clj949-patch-v2.txt dated Aug 30 2013 contains 2 commits. The first commit is the same as Brian Taylor's 0001-let-undeclared-exceptions-continue-unchecked.patch. The second commit removes all remaining occurrences of "throw sneakyThrow(e)" that can be removed without causing Java compilation errors.
clj949-patch-v2-ignore-whitespace.txt is nearly identical, and is included only for ease of review. It was produced with no lines containing only whitespace changes, using the command below, since there are many lines that were in try blocks that were only changed by removing a leading tab character:
git format-patch -w master --stdout

I have attached a slightly modified version of clj949-patch-v2.txt that should not have those warnings when applied.

FYI, the warnings were because I removed one leading tab from some Java source lines that had several tabs after the code on that line, just before the newline. Those tabs were there in the original Compiler.java lines being modified. git doesn't have any problem with a deleted line having such trailing whitespace, but it warns about adding lines with trailing whitespace.

Andy Fingerhut
added a comment - 03/Sep/13 12:12 AM I have attached a slightly modified version of clj949-patch-v2.txt that should not have those warnings when applied.
FYI, the warnings were because I removed one leading tab from some Java source lines that had several tabs after the code on that line, just before the newline. Those tabs were there in the original Compiler.java lines being modified. git doesn't have any problem with a deleted line having such trailing whitespace, but it warns about adding lines with trailing whitespace.

Alex, do you know whether you will want all screened patches to be free of trailing whitespace warnings, and other such whitespace warnings?

If so, I can try to be proactive in finding and fixing such patches before you come across them. It is a small change to my code for checking whether patches are prescreened, to also check whether they have these warnings when the patch is applied.

Alternately, it is also easy to disable trailing whitespace warnings in git. If you think it is OK for patches to add lines with trailing whitespace, I can document that on the wiki.

Andy Fingerhut
added a comment - 03/Sep/13 1:34 AM - edited Alex, do you know whether you will want all screened patches to be free of trailing whitespace warnings, and other such whitespace warnings?
If so, I can try to be proactive in finding and fixing such patches before you come across them. It is a small change to my code for checking whether patches are prescreened, to also check whether they have these warnings when the patch is applied.
Alternately, it is also easy to disable trailing whitespace warnings in git. If you think it is OK for patches to add lines with trailing whitespace, I can document that on the wiki.

Honestly, I can't say I understood all the details of what the warning meant. Seeing your explanation, it doesn't seem like a big deal but it certainly seems better to clean up the whitespace as we patch. Warnings are something that slows down evaluation of the patch b/c you have to understand whether the warning is ok, so in general I'd prefer not to see them.

Alex Miller
added a comment - 03/Sep/13 7:50 AM Honestly, I can't say I understood all the details of what the warning meant. Seeing your explanation, it doesn't seem like a big deal but it certainly seems better to clean up the whitespace as we patch. Warnings are something that slows down evaluation of the patch b/c you have to understand whether the warning is ok, so in general I'd prefer not to see them.

Got it, Alex. My program for prescreening patches now includes some output that tells me which patches have warnings when they are applied with the "git am ..." command. For each of those, it gives a different status depending upon how far the ticket is in the workflow: "fix now" for patches on tickets that are already Vetted and scheduled for the next release, "fix soon" for tickets that are only Vetted, and "fix later" for all pre-Vetted tickets.

Thus even though there are a couple dozen patches with these warnings now, almost all of them are for pre-Vetted tickets, so not urgent. I can be reminded of the patches needing cleaning-up gradually, as they are Vetted or I have spare time.

Andy Fingerhut
added a comment - 04/Sep/13 6:40 AM Got it, Alex. My program for prescreening patches now includes some output that tells me which patches have warnings when they are applied with the "git am ..." command. For each of those, it gives a different status depending upon how far the ticket is in the workflow: "fix now" for patches on tickets that are already Vetted and scheduled for the next release, "fix soon" for tickets that are only Vetted, and "fix later" for all pre-Vetted tickets.
Thus even though there are a couple dozen patches with these warnings now, almost all of them are for pre-Vetted tickets, so not urgent. I can be reminded of the patches needing cleaning-up gradually, as they are Vetted or I have spare time.