<< Back to previous view

[CLJ-949] let undeclared exceptions continue unchecked Created: 07/Mar/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Backlog
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Brian Taylor Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-let-undeclared-exceptions-continue-unchecked.patch     File clj949-patch-v2.diff     Text File clj949-patch-v2-ignore-whitespace.txt     Text File clj949-patch-v2.txt    
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 ...'



 Comments   
Comment by Andy Fingerhut [ 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.

Comment by Stuart Sierra [ 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

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() {
Comment by Andy Fingerhut [ 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

Comment by Alex Miller [ 02/Sep/13 11:41 PM ]

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.
Comment by Andy Fingerhut [ 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.

Comment by Andy Fingerhut [ 03/Sep/13 1:34 AM ]

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.

Comment by Alex Miller [ 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.

Comment by Andy Fingerhut [ 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.

Comment by Alex Miller [ 27/Sep/13 5:14 PM ]

Moving to Vetted for re-screening.

Comment by Stuart Sierra [ 11/Oct/13 6:27 PM ]

Screened.

Generated at Fri Aug 29 11:06:56 CDT 2014 using JIRA 4.4#649-r158309.