[CLJ-976] Implement reader literal and print support for PersistentQueue data structure Created: 27/Apr/12 Updated: 06/Apr/13 |
|
| Status: | Open |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | Release 1.5 |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Fogus | Assignee: | Fogus |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | data-structures, queue, reader, tagged-literals | ||
| Attachments: |
|
| Patch: | Code and Test |
| Approval: | Test |
| Description |
|
Clojure's PersistentQueue structure has been in the language for quite some time now and has found its way into a fair share of codebases. However, the creation of queues is a two step operation often of the form: (conj clojure.lang.PersistentQueue/EMPTY :a :b :c) ;=> #<PersistentQueue clojure.lang.PersistentQueue@78d5f6bc> A better experience might be the following: #queue [:a :b :c] ;=> #queue [:a :b :c] (pop #queue [:a :b :c]) ;=> #queue [:b :c] This syntax is proposed and discussed in the Clojure-dev group at https://groups.google.com/forum/?fromgroups#!topic/clojure-dev/GQqus5Wycno Open question: Should the queue literal's arguments eval? The implications of this are illustrated below: ;; non-eval case #queue [1 2 (+ 1 2)] ;=> #queue [1 2 (+ 1 2)] ;; eval case #queue [1 2 (+ 1 2)] ;=> #queue [1 2 3] The answer to this open question will determine the implementation. |
| Comments |
| Comment by Steve Miner [ 27/Apr/12 10:18 AM ] |
|
I think the non-eval behavior would be consistent with the other reader literals in Clojure 1.4. It's definitely better for interop where some other language implementation could be expected to handle a few literal representations, but not the evaluation of Clojure expressions. Use a regular function if the args need evaluation. |
| Comment by Chas Emerick [ 27/Apr/12 10:19 AM ] |
|
The precedent of records seems relevant: => (defrecord A [b])
user.A
=> #user.A[(+ 4 5)]
#user.A{:b (+ 4 5)}
=> #user.A{:b (+ 4 5)}
#user.A{:b (+ 4 5)}
This continues to make sense, as otherwise queues would need to print with an extra (quote …) form around lists — which records neatly avoid: => (A. '(+ 4 5))
#user.A{:b (+ 4 5)}
Does this mean that a queue fn (analogous to vector, maybe) will also make an appearance? It'd be handy for HOF usage. |
| Comment by Fogus [ 27/Apr/12 11:00 AM ] |
|
Added a patch for the tagged literal support ONLY. This is only one part of the total solution. This provides the read-string and printing capability. I'd like more discussion around the eval side before I get dive into the compiler. |
| Comment by Paul Michael Bauer [ 27/Apr/12 6:45 PM ] |
|
In addition to Chas' observations on consistency with record literals, would eval in queue literals open up the same security hole as #=, needing to respect *read-eval*? |
| Comment by Fogus [ 04/May/12 1:14 PM ] |
|
Evalable queue literal support. |
| Comment by Andy Fingerhut [ 10/May/12 5:54 PM ] |
|
Neither of the patches CLJ-976-queue-literal-tagged-parse-support-only.diff dated Apr 27, 2012 nor CLJ-976-queue-literal-eval.diff dated May 4, 2012 apply cleanly to latest master as of May 10, 2012. |
| Comment by Fogus [ 11/May/12 10:15 AM ] |
|
Updated patch file to merge with latest master. |
| Comment by Fogus [ 20/Jul/12 1:14 PM ] |
|
New patch with support fixed for syntax-quote. |
| Comment by Stuart Sierra [ 17/Aug/12 12:41 PM ] |
|
Patch does not apply as of commit f5f4faf95051f794c9bfa0315e4457b600c84cef |
| Comment by Fogus [ 17/Aug/12 3:06 PM ] |
|
Weird. I was able to download the CLJ-976-queue-literal-eval-and-synquote.diff patch and apply it to HEAD as of just now (f5f4faf95051f794c9bfa0315e4457b600c84cef). There were whitespace warnings, but the patch applied, compiles and passes all tests. |
| Comment by Andy Fingerhut [ 17/Aug/12 7:29 PM ] |
|
With latest head I was able to successfully apply patch CLJ-976-queue-literal-eval-and-synquote.diff with this command: git am --keep-cr -s < CLJ-976-queue-literal-eval-and-synquote.diff with some warnings, but successfully applied. If I try it without the --keep-cr option, the patch fails to apply. I believe this is often a sign that either one of the files being patched, or the patch itself, contains CR/LF line endings, which some of the Clojure source files definitely do. The command above (with --keep-cr) is currently the one recommended for applying patches on page http://dev.clojure.org/display/design/JIRA+workflow in section "Screening Tickets". I added the suggested --keep-cr option after running across another patch that applied with the option, but not without it. |
| Comment by Andy Fingerhut [ 28/Aug/12 5:45 PM ] |
|
Presumptuously changing Approval from Incomplete back to Test, since the latest patch does apply cleanly if --keep-cr option is used. |
| Comment by Rich Hickey [ 08/Sep/12 6:48 AM ] |
|
this needs more time |
| Comment by Fogus [ 18/Sep/12 8:15 AM ] |
|
Rich, Do you mind providing a little more detail? I would be happy to make any changes if needed. However, if it's just a matter of its relationship to EDN and/or waiting until the next release then I am happy to wait. In either case, I'd like to complete this or push it to the back of my mind. Thanks. |
| Comment by Andy Fingerhut [ 05/Oct/12 7:49 AM ] |
|
clj-976-queue-literal-eval-and-synquote-patch-v2.txt dated Oct 5 2012 is identical to Fogus's patch CLJ-976-queue-literal-eval-and-synquote.diff dated Jul 20 2012. It simply removes one line addition to clojure.iml that Rich has since added in a different commit, so that this patch now applies cleanly to latest master. |
| Comment by Andy Fingerhut [ 16/Oct/12 12:20 PM ] |
|
clj-976-queue-literal-eval-and-synquote-patch-v3.txt dated oct 16 2012 is identical to Fogus's patch CLJ-976-queue-literal-eval-and-synquote.diff dated Jul 20 2012. It simply removes one line addition to clojure.iml that Rich has since added in a different commit, so that this patch now applies cleanly to latest master. |
| Comment by Andy Fingerhut [ 20/Oct/12 12:26 PM ] |
|
Fogus, with the recent commit of a patch for |
| Comment by Steve Miner [ 06/Apr/13 8:07 AM ] |
|
Related to CLJ-1078. |
[CLJ-926] Instant literals do not round trip correctly Created: 05/Feb/12 Updated: 17/Feb/12 Resolved: 17/Feb/12 |
|
| Status: | Closed |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | Release 1.4 |
| Fix Version/s: | None |
| Type: | Defect | Priority: | Major |
| Reporter: | Cosmin Stejerean | Assignee: | Unassigned |
| Resolution: | Declined | Votes: | 0 |
| Labels: | instant, reader, tagged-literals | ||
| Attachments: |
|
| Patch: | Code and Test |
| Approval: | Screened |
| Waiting On: | Stuart Sierra |
| Description |
|
When using java.util.Date for instant literals (which is the default) instants do not round-trip properly during daylight savings. Here is an example: (read-string "#inst \"2010-11-12T13:14:15.666-06:00\"") #inst "2010-11-13T06:14:15.666+10:00" I'm currently in Melbourne, which is normally GMT+10. However, on November 12th daylight savings is in effect, so the proper GMT offset is +11. The date above is actually using the correct local time (6:14:15) but with the wrong offset. The problem is more obvious when you attempt to round-trip the instant that was read. user> #inst "2010-11-13T06:14:15.666+10:00" #inst "2010-11-13T07:14:15.666+10:00" Notice that we read 6:14am but the output was 7:14 with the same offset. Upon digging deeper the real culprit seems to be the use of String.format (through clojure.core/format) when outputting java.util.Date. Notice the following inside caldate->rfc3339 (format "#inst \"%1$tFT%1$tT.%1$tL%1$tz\"" d)) Let's compare the timezone offset in the underlying date with what is printed by %1$tz user> (def d #inst "2010-11-13T06:14:15.666+10:00") #'clojure.instant/d user> (.getHours d) 7 user> (.getTimezoneOffset d) -660 For reference, the definition of getTimezoneOffset is -(Calendar.get(Calendar.ZONE_OFFSET) + Calendar.get(Calendar.DST_OFFSET)) / (60 * 1000) So far it looks good. 6am in GMT+10 becomes 7am in GMT+11. Let's see how String.format handles it though. clojure.instant> (format "%1$tz" d) "+1000" clojure.instant> (format "%1$tT" d) "07:14:15" String.format prints the correct hour, but with the wrong offset. The recommended way for formatting dates is to use a DateFormatter. The String.format approach appears to work properly for Calendar, but not for Date. Therefore the attached patch keeps the current |
| Comments |
| Comment by Cosmin Stejerean [ 05/Feb/12 9:29 PM ] |
|
Not quite sure how to create a repeatable test for this since the issue depends on the local timezone. |
| Comment by Steve Miner [ 06/Feb/12 8:33 AM ] |
|
java.util.TimeZone/setDefault could be used for testing in various timezones. |
| Comment by Steve Miner [ 06/Feb/12 8:37 AM ] |
|
Regarding the patch: SimpleDateFormat is a relatively heavy-weight object, so it seems bad to allocate one every time you print a Date. Unfortunately, SimpleDateFormat is not thread-safe, so you can't just share one. The Java world seems to agree that you should use JodaTime instead, but if you're stuck with the JDK, you need to have a ThreadLocal SimpleDateFormat. I'm working on my own patch along those lines. |
| Comment by Fogus [ 06/Feb/12 7:38 PM ] |
|
Excellent analysis. I'll keep track of this case and vet any patches that come along. Please do attach a regression test if possible. |
| Comment by Cosmin Stejerean [ 06/Feb/12 8:39 PM ] |
|
I attached a new patch using a SimpleDateFormat per thread using a thread local. I'll try to add some tests next. |
| Comment by Cosmin Stejerean [ 06/Feb/12 10:42 PM ] |
|
A combined patch that uses a thread local SimpleDateFormat, tests round-tripping at a known daylight savings point (by overriding the default timezone) and checks round tripping at multiple points throughout the year (every hour of the first 4 weeks of every month of the year). Steve, thanks for the suggestions on using a thread local and TimeZone/setDefault. |
| Comment by Steve Miner [ 07/Feb/12 1:32 PM ] |
|
I filed |
| Comment by Fogus [ 07/Feb/12 3:11 PM ] |
|
Thanks for the regression test also. I'll vett this patch ASAP. |
| Comment by Fogus [ 17/Feb/12 1:24 PM ] |
|
Seems reasonable to me. |
| Comment by Stuart Sierra [ 17/Feb/12 1:46 PM ] |
|
Vetted. Will apply later. |
| Comment by Stuart Sierra [ 17/Feb/12 1:56 PM ] |
|
Not Vetted. Test. That thing. |
| Comment by Stuart Sierra [ 17/Feb/12 2:10 PM ] |
|
No, it's "Screened," not "Test." Somebody save me. |
| Comment by Stuart Sierra [ 17/Feb/12 3:55 PM ] |
|
Superseded by |