<< Back to previous view

[CLJ-1271] Reduce protocol callsite overhead Created: 30/Sep/13  Updated: 08/Oct/13  Resolved: 08/Oct/13

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

Placeholder for Rich to work on the unused instance member fields emitted by emitProto() in the compiler.






[CLJ-1557] Nested reduced is broken Created: 09/Oct/14  Updated: 10/Oct/14  Resolved: 10/Oct/14

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File re-reduced.diff    
Patch: Code
Approval: Vetted

 Description   

Re-reduced from composed transformation functions:

  • re-wraps the Reduced when it should not (take)
  • forget to unwrap the Reduced (partition-by, partition-all)
; nested reduced
=> (transduce (comp (take 1)) conj [:a])
[:a]
=> (transduce (comp (take 1) (take 1)) conj [:a])
#<Reduced@65979031: [:a]>
=> (transduce (comp (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@fcbc8d1: #<Reduced@60bea99a: [:a]>>
=> (transduce (comp (take 1) (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@6e9915bb: #<Reduced@5c712302: #<Reduced@472b9f70: [:a]>>>
 
; problems not appearing in all contexts
; not ok with transduce
=> (transduce (comp (partition-by keyword?) (take 1)) conj [] [:a])
#<Reduced@5156c42e: [[:a]]>
; but ok with sequence
=> (sequence (comp (partition-by keyword?) (take 1)) [:a])
([:a])
; well, not always
=> (sequence (comp (partition-by keyword?) (take 1)  (partition-by keyword?) (take 1)) [:a])
ClassCastException clojure.lang.Reduced cannot be cast to clojure.lang.LazyTransformer  clojure.lang.LazyTransformer$Stepper$1.invoke (LazyTransformer.java:104)

See also: https://groups.google.com/d/msg/clojure-dev/cWzMS_qqgcM/7IAhzMKzVigJ



 Comments   
Comment by Ghadi Shayban [ 09/Oct/14 11:11 PM ]

Same with partition-all

(transduce (comp (take 1) (partition-all 3) (take 1)) conj [] (range 15))
 #<Reduced@84f8976: [[0]]>
Comment by Christophe Grand [ 10/Oct/14 5:50 AM ]

patch for take, partition-by and partition-all





[CLJ-1197] Allow fold to parallelize over lazy sequences Created: 10/Apr/13  Updated: 27/Sep/13  Resolved: 27/Sep/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Paul Butcher Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: reducers

Attachments: File foldable-seq.diff    
Patch: Code

 Description   

This patch implements foldable-seq, which allows fold to parallelize over a lazy sequence. See this conversation on the Clojure mailing list:

https://groups.google.com/forum/#!msg/clojure/8RKCjF00ukQ/b5mmmOB5Uh4J

The patch is code only, sadly. No tests because I've not been able to find any existing tests for fold:

https://groups.google.com/d/msg/clojure-dev/plQ16L1_FC0/CIyMVIgSZkkJ

However, I have tested it in a separate project successfully.



 Comments   
Comment by Stuart Halloway [ 27/Sep/13 3:12 PM ]

Hi Paul,

Seqs are fundamentally not foldable. That said, what you seem to be trying to do (partition a seq into foldable subjobs) is straightforward in Clojure, see

https://github.com/stuarthalloway/exploring-clojure/blob/master/examples/exploring/reducing_apple_pie.clj?#L62-73

That said, if the input is truly arriving sequentially, pmap or some variant may do as well or better, e.g.

https://github.com/stuarthalloway/exploring-clojure/blob/master/examples/exploring/reducing_apple_pie.clj?#L77-83





[CLJ-1165] Forbid varargs defprotocol/definterface method declarations because those cannot be defined anyway Created: 15/Feb/13  Updated: 28/Oct/13  Resolved: 28/Oct/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Tassilo Horn Assignee: Stuart Halloway
Resolution: Duplicate Votes: 0
Labels: protocols

Attachments: Text File 0001-Protocol-interface-method-declarations-don-t-allow-f.patch    
Patch: Code

 Description   

Protocol, interface method declarations don't allow for varags. Currently, for example

  (defprotocol FooBar
    (foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that happens to be
named & without special meaning. But clearly, the user wanted to specify a
varags parameter here. The same applies to definterface.

Similarly, providing method implementations via defrecord, deftype, and reify
don't allow for varags (but dynamic extensions via extend do).

So this patch makes defprotocol and definterface throw an
IllegalArgumentException if a user tries to use varargs in method signatures.

Similarly, defrecord, deftype, and reify throw an IllegalArgumentException if
any method implementation arglist contains a varargs argument.

This patch is a cut-down variant of my patch to http://dev.clojure.org/jira/browse/CLJ-1024
which has been reverted shortly before Clojure 1.5 was released. The CLJ-1024 patch
was the same as this one, but it has also forbidden destructuring in defprotocol and
definterface. This was a bit too much, because although destructuring has no
semantic meaning with method declarations, it still can serve a documentation purpose.

This has been discussed on the list: https://groups.google.com/d/topic/clojure-dev/qjkW-cv8nog/discussion



 Comments   
Comment by Stuart Halloway [ 29/Mar/13 5:27 AM ]

I think that this patch would be much more helpful to users if it reported the problem form (both name and params).

(And I wonder if we should be using ex-info for all errors going forward.)

Comment by Tassilo Horn [ 31/Mar/13 5:17 AM ]

New version of the patch that mentions both method name and argument vector, and uses ex-info as Stu suggested.

Comment by Andy Fingerhut [ 04/Apr/13 7:24 PM ]

Presumuptuously changing Approval from Incomplete back to None, since the reason for marking it Incomplete seems to have been addressed with a new patch.

Comment by Andy Fingerhut [ 25/Oct/13 6:29 PM ]

I have not investigated the reason yet, but patch 0001-Protocol-interface-method-declarations-don-t-allow-f.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Comment by Tassilo Horn [ 28/Oct/13 2:18 AM ]

I'm closing this issue in favor of CLJ-888 which is about the very same issue and contains a more recent patch that also contains test cases.





[CLJ-1188] Public Java API Created: 30/Mar/13  Updated: 12/Apr/13  Resolved: 12/Apr/13

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1188.patch     Text File CLJ-1188-via-var-intern.patch     Text File CLJ-1188-wrapper-free.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Java consumers need an API into Clojure that does not drag in a ton of concrete implementation detail.

Solution: Very small API class that allows looking up Vars (returning IFn), and reading data (as edn). Uses Clojure logic where possible, e.g. Var.intern.

Current patch: CLJ-1188-via-var-intern.patch

Also considered:

  • wrapper class (inconvenient for users, wrappers anathema in Clojure)
  • common superinterface for IFn and Deref (unnecessary, might bloat vtable)

See also http://dev.clojure.org/display/design/Improvements+to+interop+from+Java



 Comments   
Comment by Kevin Downey [ 02/Apr/13 11:34 AM ]

the attached patch would turn

...
public static Var ENQUEUE = RT.var("greenmail.smtp","enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

in to something like

...
public static VRef ENQUEUE = API.vref("greenmail.smtp/enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

what is the value of VRefs over using Vars directly?

Comment by Kevin Downey [ 02/Apr/13 5:56 PM ]

using this from a java api, it looks like if the namespace the var is in is not loaded when you go to create a VRef it will return null, generally my java code that calls clojure looks something like

public static Var FOO = RT.var("namespace", "name");
public static NAMESPACE = Symbol.intern("namespace");
public static Var REQUIRE = RT.var("clojure", "require");

static {
  REQUIRE.invoke(NAMESPACE);
}

can you tell me without checking the java/jvm spec if FOO is null? does the static init block run before or after static fields are init'ed? returning null just seems like a bad idea.

Comment by Stuart Halloway [ 03/Apr/13 6:53 AM ]

Per discussion on the ticket and with Rich, the wrapper-free approach (Apr 3 patch) is preferred.

Comment by Stuart Halloway [ 03/Apr/13 7:03 AM ]

Hi Kevin,

The purpose of not returning Vars is outlined in the design discussion. That said, it is possible to return IFn, which does not drag in too much implementation detail (just a javadoc config tweak, see CLJ-1190). Today's patch returns IFn, which addresses the wrapper ickiness demonstrated by your code examples.

Java static initializers run in lexical order, and I trust Java programmers to know Java.

I can think of several options other than returning null when a var is not available, and they are all complecting, inconsistent with Clojure, or both.

Comment by Kevin Downey [ 03/Apr/13 12:08 PM ]

Hey,

Always returning the var is very consistent with clojure, it is what RT.var does. It is what the var lookups emitted by the compiler do. RT.var is most likely the point through which most java that calls clojure goes at the moment.

As to "hiding" vars, how about creating a clojure.lang.ILink interface with both deref() and fn(), have Var implement it. The previous discussion I see linked all seems to presume hiding Var without discussion of why it should be hidden. What I see Rich saying is:

So RT.var is the right idea. It would be nice to hide the Var class,
but unfortunately we can't make ClojureAPI.var() return both IFn and
IDeref without inventing a common subinterface. There are other
similar details.

and

We will need an interface unifying IDeref and IFn for the return type
of API.var()

It seems like Rich is suggesting that ILink or whatever should also bring in IFn, but I wonder if having a fn() that does the cast for you is an acceptable alternative to that.

Comment by Rich Hickey [ 12/Apr/13 8:24 AM ]

The API should be called var, not fn, and should return IFn. Also, I really want the logic of RT.var (i.e. Var.intern) to be used, not this new logic. Please find another way to handle the string/symbol support.





[CLJ-1174] Website doc link for 1.4 api docs returns a 404 Created: 05/Mar/13  Updated: 05/Mar/13  Resolved: 05/Mar/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Ed O'Loughlin Assignee: Tom Faulhaber
Resolution: Completed Votes: 0
Labels: None
Environment:

All



 Description   

The API docs for Clojure 1.4 (http://clojure.github.com/clojure/branch-clojure-1.4.0/index.html), linked to from http://clojure.github.com/clojure/, returns a 404.

I logged it under this category because I can't see anywhere else to log bugs about clojure.org.



 Comments   
Comment by Tom Faulhaber [ 05/Mar/13 8:29 PM ]

Fixed. Thanks for pointing this out.





[CLJ-873] Allow the function / to be referred to in namespaces other than clojure.core Created: 10/Nov/11  Updated: 28/Jul/13  Resolved: 28/Jul/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.1, Release 1.2, Release 1.3, Release 1.4
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Chris Gray Assignee: Unassigned
Resolution: Completed Votes: 7
Labels: None

Attachments: Text File 0001-Fix-CLJ-873-for-EdnReader-too.patch     Text File clj-873-namespace-divides-patch.txt     File namespace-divides.diff    
Patch: Code
Approval: Ok

 Description   

The attached patch gives the programmer the option of referring to the division function in namespaces other than just clojure.core. For example,

(ns foo
(:require [cljs.core :as core]))
(apply core// '(1 2 3))

The above lines do not compile without this patch.



 Comments   
Comment by Chris Gray [ 10/Nov/11 9:50 AM ]

I have signed the CA and it is in the mail.

Comment by Chris Gray [ 20/Nov/11 6:21 PM ]

My CA has now been applied. This patch is quite simple – can someone have a look at it please?

Comment by Alex Miller [ 09/Dec/11 9:34 AM ]

FYI, I have run into this in actual code as well (implementing a query language function library).

Comment by Andy Fingerhut [ 24/Feb/12 8:00 PM ]

clj-873-namespace-divides-patch.txt is same as Chris's, just updated to apply cleanly to latest master as of Feb 24, 2012.

The test he added does fail without the code fix, and passes with it. He is now on the list of contributors.

Comment by Chris Gray [ 30/Mar/12 1:11 PM ]

A short further discussion of this patch appeared here: http://groups.google.com/group/clojure-dev/browse_thread/thread/f095980802a82747/b723726c77c1ec64

Also, I assume this bug is what is referred to in Clojurescript's core.cljs, where it says ";; FIXME: waiting on cljs.core//"

Comment by Stuart Halloway [ 22/Oct/12 7:12 PM ]

Thanks all. It is nice to have supporting real-world stories such as Alex's in the comments.

Comment by Andy Fingerhut [ 22/Oct/12 7:19 PM ]

I should have added a comment here a while back if it would have helped, but David Nolen's CLJ-930 was closed as a duplicate of this one.

Comment by Brandon Bloom [ 02/Jan/13 12:49 AM ]

This also affects a two of my libraries: 1) CSS generation library I'm working on, which wants to be able to do division with pixels and other units. 2) Factjor which defines binary math operators against a stack.

Comment by Nicola Mometto [ 24/May/13 8:39 AM ]

clojure.lang.EdnReader should get patched aswell.

Comment by Nicola Mometto [ 25/May/13 11:48 AM ]

I'm reopening it, attaching a patch for EdnReader

Comment by Andy Fingerhut [ 25/May/13 12:37 PM ]

Nicola, I noticed yesterday that LispReader.java still contains values SLASH and CLOJURE_SLASH that are no longer used after the patch was applied yesterday for this ticket. Would you mind including the removal of those in your patch, too?

Comment by Nicola Mometto [ 25/May/13 2:05 PM ]

Andy, I've updated 0001-Fix-CLJ-873-for-EdnReader-too.patch to remove SLASH and CLOJURE_SLASH as you suggested.

Comment by Alex Miller [ 28/Jul/13 10:22 PM ]

Since the last patch on this came in after the change had been applied, I moved the cleanup patch to CLJ-1238.

Comment by Alex Miller [ 28/Jul/13 10:23 PM ]

Re-resolving. Moved extra late patch to new ticket - CLJ-1238.





[CLJ-1220] instance? should either verify all operands or throw if more than one passed Created: 19/Jun/13  Updated: 02/Sep/13  Resolved: 02/Sep/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Irakli Gozalishvili Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

(instance? Number 1) ;; => true
(instance? Number "a") ;; => false
(instance? Number 1 "a") ;; => true

I find behavior of the last expression very surprising, I would
expect it to either desugar to logical "and" over all operands:

(and (instance? Number 1) (instance? Number "a") ...)

Or throw "Wrong number of args (3) passed to instance?" exception.



 Comments   
Comment by Andy Fingerhut [ 19/Jun/13 5:32 PM ]

Irakli, one of the patches for CLJ-1171 addresses this issue by causing (instance? Number 1 "a") to throw a Wrong number of args (3) passed to core$instance-QMARK- ArityException.

Comment by Alex Miller [ 02/Sep/13 7:52 PM ]

Fixed by CLJ-1171. In 1.6 master, now gets:

user=> (instance? Number 1 "a")
ArityException Wrong number of args (3) passed to: core/instance?  clojure.lang.AFn.throwArity (AFn.java:436)




[CLJ-420] Undefined symbols raise exceptions with line/column number of enclosing expression Created: 08/Aug/10  Updated: 28/Oct/13  Resolved: 25/Oct/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs, reader

Attachments: Text File CLJ-420-2.patch     Text File CLJ-420-3.patch     Text File CLJ-420-4.patch     Text File CLJ-420.patch    
Patch: Code
Approval: Screened

 Description   

Certain kinds of errors in loaded source files are coming back tagged with the correct source file, but an incorrect line:column number. This seems to happen when unknown symbols occur by themselves, not called as a function.

The general pattern appears to be that an undefined symbol is reported with a line number of the beginning of its nearest enclosing expression. If the undefined symbol appears at the top level of a file, it is reported with line:column number 0:0, or line:column number of REPL input, if loaded from a REPL. The behavior is different in a Leiningen REPL. If the undefined symbol appears at the top level of a file, it is reported with line:column number 1:1.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1)

Patch: CLJ-420-3.patch

Approach: Capture line and column metadata for symbols in the LispReader. A few tests were adjusted to ignore line and col metadata for protocol symbols which now have them.

Screened by: Alex Miller

Background: Clojure Google group thread when this issue was originally reported in 2010 against Clojure 1.2: http://groups.google.com/group/clojure/browse_frm/thread/beb36e7228eabd69/a7ef16dcc45834bc?hl=en#a7ef16dcc45834bc



 Comments   
Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/420

Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

stu said: Updating tickets (#427, #426, #421, #420, #397)

Comment by Assembla Importer [ 28/Sep/10 9:59 PM ]

stu said: Updating tickets (#429, #437, #397, #420)

Comment by Alex Miller [ 10/Aug/13 12:20 AM ]

Based on the updated information (which is really a totally different issue), I have reduced priority from Major to Minor, removed the fix version and sent this back down to Triaged for Rich to take another look.

Comment by Paavo Parkkinen [ 28/Sep/13 6:58 AM ]

Just noticed that when I reproduce this with current code from Github, I get the same behaviour that was in the original report.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp target/clojure-1.6.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)    
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 
Comment by Paavo Parkkinen [ 28/Sep/13 7:23 AM ]

I also get the original behaviour with 1.5.1.

$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 

Could be lein is mixing it up. I also get the behaviour in the description if I do it with lein.

Comment by Andy Fingerhut [ 28/Sep/13 11:22 PM ]

Paavo, I think you are correct that I was getting different behavior than the original description because I was using Leiningen, rather than straight Clojure, and that the original description's behavior is still true with the latest Clojure master when Leiningen is not used. My bad for changing the description unnecessarily. Would you be willing to correct it?

Comment by Paavo Parkkinen [ 29/Sep/13 7:21 AM ]

Corrected description.

Comment by Paavo Parkkinen [ 06/Oct/13 5:20 AM ]

I have a fix for This bug. I will attach it to the ticket, but it is not supposed to be considered for approval, as it is clearly not finished (i.e. cleaned up) yet. If there is a better way to solicit comments for a proposed fix, please let me know.

For the fix, I simply copied the line:column number tracking code from ListReader.invoke (also used in others) into LispReader.read for Symbols. Only issues I see is that some test cases get confused because some meta data contains line:column info where it didn't use to.

I also tried to fix the line:column numbering for files like test3 and test4 below, and the fix is in the patch file, but commented out, because the fix for that causes some compile problems. I have not yet tracked down what the cause of the problems is. I suppose the correct procedure is to leave it out for now, and possibly open a new ticket for it?

$ cat test3.clj 

(
bla)
$ cat test4.clj 

(print
bla)

If anyone has any comments or suggestions, I would be very happy to hear them. In the mean time I'll start checking out the failing test cases, and seeing if I can correct them to work with the fix. I'm not sure how I should go about testing the fix itself.

Also, if the comments here is not the correct place for this discussion, please let me know and I will move it elsewhere (clojure-dev).

P.s. I suppose for test2, the column number should be 2, not 1. Not sure if that's a big deal or not.

Comment by Alex Miller [ 17/Oct/13 11:04 PM ]

A few things here:

1) I think you should drop the commented Compiler changes and just focus on the reader changes.
2) This patch has a number of test failures. In particular, protocol symbols now have line and column metadata. That's potentially quite useful but I haven't fully contemplated the ramifications.

Moving to Incomplete for now.

Comment by Paavo Parkkinen [ 19/Oct/13 9:40 PM ]

I attached a new version of patch, which gets rid of the Compiler changes, and fixes the test cases by simply ignoring the additional line/column metadata. I'm not aware of any way to reliably know what the values are going to be so they could be checked.

Or course there might be code that relies on the metadata not including line/column info, and that code will break with the patch. I can't really think of any reason why anyone would do that, and can't estimate how many people might be affected. After the patch people will probably start using the new metadata, and after that it will be difficult to get rid of, if needed in the future.

There is, I think, also potential performance effects. I believe the metadata is generated at compile time, but will be available at runtime, which will mean at least a minor increase in footprint, and possibly CPU as well. Again, I'm not that familiar with how the runtime works that I could really estimate that.

Of course there may also be other effects that I can't imagine right now.

Comment by Alex Miller [ 20/Oct/13 5:05 PM ]

I updated with a new patch that fixes some whitespace errors and removes the CLJ-420 comments which don't seem very helpful to me. Marking screened.

Comment by Rich Hickey [ 25/Oct/13 7:51 AM ]

this breaks tests, and will also break a ton of code not expecting this new metadata

Comment by Paavo Parkkinen [ 28/Oct/13 7:44 AM ]

Attached a patch (CLJ-420-4.patch) which fixes this without introducing the new metadata into symbols. Instead of using metadata to pass the line/column info from the reader to the compiler, I just store it in fields in the Symbol class.

Comment by Andy Fingerhut [ 28/Oct/13 1:31 PM ]

Paavo, I am not a Clojure screener, and cannot say any of this with authority to back it up, but Rich chose to close this ticket as declined, meaning that it would take some convincing for him to consider a ticket for it again. I am not saying you can't attach all of the patches you want to this ticket, but they might not go anywhere.

Comment by Alex Miller [ 28/Oct/13 2:33 PM ]

+1 to Andy's comment.

Further, I don't think the replacement patch respects the immutability and immutability concerns of interned Symbols - setting the line and column is a big race condition in the patch as is. IF this path were going to be acceptable, line and column would need to be final fields set in the Symbol constructor. However, it seems to me that you could easily have two symbols sharing the same interned Symbol that perhaps were created in different locations.

An additional item to consider is the additional memory overhead of carrying two additional ints on every Symbol. In general, it does not seem to me that this path provides enough value per overhead/effort so I would think I would recommend letting it go OR filing a new ticket.

Comment by Paavo Parkkinen [ 28/Oct/13 7:10 PM ]

I didn't realize the ticket was closed. Thanks for pointing that out.

And thanks for taking the time to still take a look at and comment on my patch, Alex. It's good advice.





[CLJ-1374] Make PersistentQueue implement List Created: 09/Mar/14  Updated: 31/Mar/14  Resolved: 31/Mar/14

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: File clj-1374-1.diff    
Patch: Code and Test

 Description   

Most ordered Clojure collections like lists, vectors, and lazy seqs implement java.util.List, and thus .equals can be true between values of those types and other collections implementing java.util.List, like java.util.Vector and java.util.ArrayList.

Clojure PersistentQueue seems to be the odd man out here, in that it implements Collection but not List, and thus while it can be .equals to Clojure lists, vectors, and lazy seqs, it cannot be .equals to other collections implementing java.util.List.

user=> (instance? java.util.List '())
true
user=> (instance? java.util.List (lazy-seq))
true
user=> (instance? java.util.List [])
true
user=> (instance? java.util.List (vector-of :long))
true
user=> (instance? java.util.List clojure.lang.PersistentQueue/EMPTY)
false

user=> (= '() (java.util.ArrayList.))
true
user=> (= (lazy-seq) (java.util.ArrayList.))
true
user=> (= [] (java.util.ArrayList.))
true
user=> (= (vector-of :long) (java.util.ArrayList.))
true
user=> (= clojure.lang.PersistentQueue/EMPTY (java.util.ArrayList.))
false


 Comments   
Comment by Andy Fingerhut [ 09/Mar/14 6:29 PM ]

Patch clj-1374-1.diff is written assuming that CLJ-1372 patch clj-1372-2.diff or very similar has been committed, because of the tests modified, not because of the change of PersistentQueue to implement the java.util.List interface. I can update this patch as desired if that change does not go in.

Comment by Andy Fingerhut [ 09/Mar/14 6:33 PM ]

Ugh. The subject is definitely a duplicate of CLJ-1059, which I should have checked for before creating this ticket. I will compare patches to see how the approaches compare. Mine is probably a poor substitute for that one, but the tests I add may still be useful to keep in a patch for CLJ-1059.

Comment by Andy Fingerhut [ 31/Mar/14 5:29 PM ]

Problem description is a duplicate of CLJ-1059. Even the patch (independently developed) is nearly the same as the patch with a name beginning with "001" attached to CLJ-1059.





[CLJ-999] Wrong link in gh-pages index (api-index.html) Created: 18/May/12  Updated: 26/Jul/13  Resolved: 20/May/13

Status: Resolved
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Bogdan Popescu Assignee: Tom Faulhaber
Resolution: Completed Votes: 0
Labels: docs, documentation

Patch: Code

 Description   

The api-index.html includes wrong links for the following:

  • All entries for all listed as part of clojure.test.tap
  • All entries for all listed as part of clojure.test.junit
  • All entries for all listed as part of clojure.core.protocols

The links point to pages that do not exist. The problem is that the documentation for those entries is on a "parent" page, for example, the link clojure.core.protocols-api.html#clojure.core.protocols/internal-reduce should have been clojure.core-api.html#clojure.core.protocols/internal-reduce

Not a huge bug for me, but you might want to get it fixed.

And please give my huge thanks to whoever is in charge of the documentation, I'm the developer behind Dash, a Mac OS X documentation browser, and I was in the process of creating a documentation set for Clojure, and because you guys have an index, you made my work 1000 times easier.



 Comments   
Comment by Andy Fingerhut [ 11/Mar/13 3:01 PM ]

Is this fixed now? Tom Faulhaber has regenerated the docs after the recent Clojure 1.5 release, and I think updated other things besides, so it might be.

Comment by Tom Faulhaber [ 11/Mar/13 4:43 PM ]

Nope, not fixed.

This one either slipped by me or came in right when I was changing jobs so didn't stick in my brain.

I'll take a look now. Thanks for the report, Bogdan, and thanks for the bump, Andy to get it on my radar.

Comment by Gabriel Horner [ 10/May/13 4:00 PM ]

Tom, I'm happy to help if you need it. Could you document on a wiki page how autodoc is run here? I couldn't find such a page.

Comment by Tom Faulhaber [ 20/May/13 4:18 PM ]

This is fixed with gh-pages commit 919143e (autodoc doesn't follow the regular Clojure release path since it's a website built off the source checkins).

Comment by Tom Faulhaber [ 20/May/13 4:24 PM ]

Gabriel, Thanks for the offer. I fixed this one, but may take you up on it if more come up.

There is currently no wiki page about the autodoc process but it's an excellent suggestion. I'll put it on my list to write something up. In the meantime source on the autodoc program itself is at https://github.com/tomfaulhaber/autodoc and a description of how it works is at http://tomfaulhaber.github.io/autodoc. Two caveats: (1) autodoc is currently undergoing a bunch of work (thus this bug fix) in preparation for a new release and (2) the documentation doesn't talk much about how it's used for documenting Clojure itself.





Generated at Thu Nov 20 20:49:57 CST 2014 using JIRA 4.4#649-r158309.