Clojure

let undeclared exceptions continue unchecked

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Backlog
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

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 ...'

  1. 0001-let-undeclared-exceptions-continue-unchecked.patch
    07/Mar/12 7:22 PM
    8 kB
    Brian Taylor
  2. clj949-patch-v2.diff
    22/Oct/13 9:18 AM
    19 kB
    Alex Miller
  3. clj949-patch-v2.txt
    03/Sep/13 12:12 AM
    19 kB
    Andy Fingerhut
  4. clj949-patch-v2-ignore-whitespace.txt
    30/Aug/13 9:00 PM
    14 kB
    Andy Fingerhut

Activity

Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Stuart Sierra added a comment -

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

diff --git a/src/jvm/clojure/lang/EdnReader.java b/src/jvm/clojure/lang/EdnReader.java
index 494c0b4..545fba0 100644
--- a/src/jvm/clojure/lang/EdnReader.java
+++ b/src/jvm/clojure/lang/EdnReader.java
@@ -62,12 +62,7 @@ static boolean nonConstituent(int ch){

 static public Object readString(String s, IPersistentMap opts){
        PushbackReader r = new PushbackReader(new java.io.StringReader(s));
-       try {
        return read(r, opts);
-       }
-       catch(Exception e) {
-       throw Util.sneakyThrow(e);
-       }
 }

 static boolean isWhitespace(int ch){
diff --git a/src/jvm/clojure/lang/Ref.java b/src/jvm/clojure/lang/Ref.java
index cf7ffa7..f687575 100644
--- a/src/jvm/clojure/lang/Ref.java
+++ b/src/jvm/clojure/lang/Ref.java
@@ -241,14 +241,7 @@ public Object call() {
 }

 public void run(){
-       try
-               {
-               invoke();
-               }
-       catch(Exception e)
-               {
-               throw Util.sneakyThrow(e);
-               }
+       invoke();
 }

 public Object invoke() {
Show
Stuart Sierra added a comment - 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
diff --git a/src/jvm/clojure/lang/EdnReader.java b/src/jvm/clojure/lang/EdnReader.java
index 494c0b4..545fba0 100644
--- a/src/jvm/clojure/lang/EdnReader.java
+++ b/src/jvm/clojure/lang/EdnReader.java
@@ -62,12 +62,7 @@ static boolean nonConstituent(int ch){

 static public Object readString(String s, IPersistentMap opts){
        PushbackReader r = new PushbackReader(new java.io.StringReader(s));
-       try {
        return read(r, opts);
-       }
-       catch(Exception e) {
-       throw Util.sneakyThrow(e);
-       }
 }

 static boolean isWhitespace(int ch){
diff --git a/src/jvm/clojure/lang/Ref.java b/src/jvm/clojure/lang/Ref.java
index cf7ffa7..f687575 100644
--- a/src/jvm/clojure/lang/Ref.java
+++ b/src/jvm/clojure/lang/Ref.java
@@ -241,14 +241,7 @@ public Object call() {
 }

 public void run(){
-       try
-               {
-               invoke();
-               }
-       catch(Exception e)
-               {
-               throw Util.sneakyThrow(e);
-               }
+       invoke();
 }

 public Object invoke() {
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - 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
Hide
Alex Miller added a comment -

clj949-patch-v2.txt had warnings when I applied it:

$ git am --keep-cr -s --ignore-whitespace < ~/Downloads/clj949-patch-v2.txt
Applying: let undeclared exceptions continue unchecked
Applying: CLJ-949: let more undeclared exceptions continue unchecked
/Users/alex/code/clojure/.git/rebase-apply/patch:189: trailing whitespace.
					gen.visitInsn(I2L);
/Users/alex/code/clojure/.git/rebase-apply/patch:194: trailing whitespace.
					gen.visitInsn(F2D);
/Users/alex/code/clojure/.git/rebase-apply/patch:204: trailing whitespace.
					gen.visitInsn(D2F);
warning: 3 lines add whitespace errors.
Show
Alex Miller added a comment - clj949-patch-v2.txt had warnings when I applied it:
$ git am --keep-cr -s --ignore-whitespace < ~/Downloads/clj949-patch-v2.txt
Applying: let undeclared exceptions continue unchecked
Applying: CLJ-949: let more undeclared exceptions continue unchecked
/Users/alex/code/clojure/.git/rebase-apply/patch:189: trailing whitespace.
					gen.visitInsn(I2L);
/Users/alex/code/clojure/.git/rebase-apply/patch:194: trailing whitespace.
					gen.visitInsn(F2D);
/Users/alex/code/clojure/.git/rebase-apply/patch:204: trailing whitespace.
					gen.visitInsn(D2F);
warning: 3 lines add whitespace errors.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Andy Fingerhut added a comment - - 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.

Show
Andy Fingerhut added a comment - - 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

Moving to Vetted for re-screening.

Show
Alex Miller added a comment - Moving to Vetted for re-screening.
Hide
Stuart Sierra added a comment -

Screened.

Show
Stuart Sierra added a comment - Screened.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: