Clojure

Read/print round-trip for +/-Infinity and NaN

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.4
  • Fix Version/s: Release 1.9
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

A few float-related forms (namely, Double/POSITIVE_INFINITY, Double/NEGATIVE_INFINITY, Double/NaN) are not eval-able after a round-trip via

(read-string (binding [*print-dup* true] (pr-str f))

The two options I see are to provide print-method implementations for these and their Float cousins, or to make Infinity, -Infinity, +Infinity, and NaN readable values. Since it sounds like edn may want to provide a spec for these values (see https://groups.google.com/d/topic/clojure-dev/LeJpOhHxESs/discussion and https://github.com/edn-format/edn/issues/2), I think making these values directly readable as already printed is preferable. Something like Double/POSITIVE_INFINITY seems too low-level from edn's perspective, as it would refer to a Java class and constant.

I'm attaching a patch implementing reader support for ##Inf, ##-Inf, ##+Inf, and ##NaN.

Patch: clj-1074-8.patch

  1. 0001-CLJ-1074-add-Inf-Inf-Inf-NaN.patch
    06/Sep/17 9:42 AM
    5 kB
    Nicola Mometto
  2. 0001-CLJ-1074-add-Inf-Inf-Inf-NaN-v2.patch
    06/Sep/17 1:55 PM
    4 kB
    Nicola Mometto
  3. 0001-Read-Infinity-and-NaN.patch
    21/Sep/12 7:13 PM
    2 kB
    Colin Jones
  4. clj-1074-2.patch
    24/Oct/16 9:43 AM
    3 kB
    Alex Miller
  5. clj-1074-3.patch
    30/Jun/17 9:03 AM
    3 kB
    Alex Miller
  6. clj-1074-5.patch
    07/Sep/17 8:20 AM
    8 kB
    Alex Miller
  7. clj-1074-6.patch
    07/Sep/17 11:47 AM
    8 kB
    Alex Miller
  8. clj-1074-7.patch
    07/Sep/17 1:31 PM
    9 kB
    Alex Miller
  9. clj-1074-8.patch
    07/Sep/17 1:44 PM
    9 kB
    Alex Miller
  10. clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch
    25/May/13 11:55 AM
    2 kB
    Nicola Mometto

Activity

Hide
Timothy Baldridge added a comment -

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Show
Timothy Baldridge added a comment - Please bring this up on clojure-dev. We'll be able to vet this ticket after that.
Hide
Colin Jones added a comment -

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Show
Colin Jones added a comment - Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?
Hide
Andy Fingerhut added a comment -

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Show
Andy Fingerhut added a comment - Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.
Hide
Nicola Mometto added a comment -

clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged

Show
Nicola Mometto added a comment - clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged
Hide
Andrew Tarzwell added a comment -

We're running into this bug now, applying the patch clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch seems to resolve it on 1.7 master, but it would be an improvement to not depend on a patched version.

Is there a fix in the works? Or a more up to date conversation on why this hasn't been addressed?

Thanks,
Andrew

Show
Andrew Tarzwell added a comment - We're running into this bug now, applying the patch clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch seems to resolve it on 1.7 master, but it would be an improvement to not depend on a patched version. Is there a fix in the works? Or a more up to date conversation on why this hasn't been addressed? Thanks, Andrew
Hide
Andy Fingerhut added a comment - - edited

Andrew, the tools.reader library provides this enhancement today, if you would prefer using unpatched software, and if it meets your needs. There are a few other small differences between it and the reader built into Clojure, which you can see near the end of its README: https://github.com/clojure/tools.reader

As far as why it hasn't been addressed yet, I think a short accurate answer is that the Clojure developers have been working on other issues, and this one has not been high enough priority to be addressed yet (disclaimer: This is in no way an official answer, just the best guess of an observer who watches this stuff too much).

I see you have voted on the ticket. Good. More votes can in some cases influence the Clojure developers to respond to a ticket earlier rather than later. You may try to persuade others to vote on it, too, if you wish.

If there is some production use of Clojure hindered by the lack of a fix, bringing this up in the Clojure or Clojure Dev Google groups couldn't hurt (signed CA on file required for Clojure Dev group membership – see http://clojure.org/contributing )

Show
Andy Fingerhut added a comment - - edited Andrew, the tools.reader library provides this enhancement today, if you would prefer using unpatched software, and if it meets your needs. There are a few other small differences between it and the reader built into Clojure, which you can see near the end of its README: https://github.com/clojure/tools.reader As far as why it hasn't been addressed yet, I think a short accurate answer is that the Clojure developers have been working on other issues, and this one has not been high enough priority to be addressed yet (disclaimer: This is in no way an official answer, just the best guess of an observer who watches this stuff too much). I see you have voted on the ticket. Good. More votes can in some cases influence the Clojure developers to respond to a ticket earlier rather than later. You may try to persuade others to vote on it, too, if you wish. If there is some production use of Clojure hindered by the lack of a fix, bringing this up in the Clojure or Clojure Dev Google groups couldn't hurt (signed CA on file required for Clojure Dev group membership – see http://clojure.org/contributing )
Hide
Andrew Tarzwell added a comment -

Andy,

Thank you for the quick response, I was unaware of tools.reader having this fixed. That should do us for now.

Show
Andrew Tarzwell added a comment - Andy, Thank you for the quick response, I was unaware of tools.reader having this fixed. That should do us for now.
Hide
Alex Miller added a comment -

Refreshed patch to apply to current master. No semantic changes, attribution retained.

Show
Alex Miller added a comment - Refreshed patch to apply to current master. No semantic changes, attribution retained.
Hide
Alex Miller added a comment -

Updated patch so it would apply cleanly, no other changes, attribution retained.

Show
Alex Miller added a comment - Updated patch so it would apply cleanly, no other changes, attribution retained.
Hide
Rich Hickey added a comment -

polluting the top-level is not an option. I propose a new dispatch macro character #, and these values: ##Inf ##-Inf ##NaN. We'll need patches for LispReader, edn reader, and the edn spec

Show
Rich Hickey added a comment - polluting the top-level is not an option. I propose a new dispatch macro character #, and these values: ##Inf ##-Inf ##NaN. We'll need patches for LispReader, edn reader, and the edn spec
Hide
Nicola Mometto added a comment -

Should we also change how +/-Inf and NaN print then?

Show
Nicola Mometto added a comment - Should we also change how +/-Inf and NaN print then?
Hide
Alex Miller added a comment -

Yes.

Show
Alex Miller added a comment - Yes.
Hide
Nicola Mometto added a comment -

Attached patch including support for ##Inf ##+Inf ##-Inf and ##NaN (I've included ##+Inf for consistency, but let me know if it's not desiderable.) I'll make a PR on the edn spec about it once I know whether or not to include ##+Inf

Show
Nicola Mometto added a comment - Attached patch including support for ##Inf ##+Inf ##-Inf and ##NaN (I've included ##+Inf for consistency, but let me know if it's not desiderable.) I'll make a PR on the edn spec about it once I know whether or not to include ##+Inf
Hide
Stuart Halloway added a comment -

Thanks Nicola!

Show
Stuart Halloway added a comment - Thanks Nicola!
Hide
Stuart Halloway added a comment -

Rich would like to have

  • remove the +Inf
  • rename SpecialSymbolReader to SymbolicValueReader
Show
Stuart Halloway added a comment - Rich would like to have
  • remove the +Inf
  • rename SpecialSymbolReader to SymbolicValueReader
Hide
Rich Hickey added a comment -

Yes, thanks Nicola. Let's go with SymbolicValueReader and not do +Inf

Show
Rich Hickey added a comment - Yes, thanks Nicola. Let's go with SymbolicValueReader and not do +Inf
Hide
Nicola Mometto added a comment -

Updated patch as per comments

Show
Nicola Mometto added a comment - Updated patch as per comments
Hide
Alex Miller added a comment -

New patch fixes printing for ##NaN, modifies the exception handling in the readers slightly, and adds some more printing tests.

Show
Alex Miller added a comment - New patch fixes printing for ##NaN, modifies the exception handling in the readers slightly, and adds some more printing tests.
Hide
Alex Miller added a comment -

Did a perf check on number printing with this:

(use 'criterium.core)				
(def v (vec (range 1000)))		 
(def vd (vec (range 0.0 1000.0 1.0)))
(bench (pr-str v))
(bench (pr-str vd))

Results:

int vector:     168.3 µs -> 231.1 µs (37% slower)
double vector:  185.2 µs -> 270.7 µs (46% slower
Show
Alex Miller added a comment - Did a perf check on number printing with this:
(use 'criterium.core)				
(def v (vec (range 1000)))		 
(def vd (vec (range 0.0 1000.0 1.0)))
(bench (pr-str v))
(bench (pr-str vd))
Results:
int vector:     168.3 µs -> 231.1 µs (37% slower)
double vector:  185.2 µs -> 270.7 µs (46% slower
Hide
Alex Miller added a comment -

Dropping a new patch that doesn't put a conditional in the Number case and instead adds a print-method for Double. Updated #s:

long vector:    168.3 µs -> 166.6 µs (wash)
double vector:  185.2 µs -> 207.6 µs (12% slower)

That seems tolerable.

Show
Alex Miller added a comment - Dropping a new patch that doesn't put a conditional in the Number case and instead adds a print-method for Double. Updated #s:
long vector:    168.3 µs -> 166.6 µs (wash)
double vector:  185.2 µs -> 207.6 µs (12% slower)
That seems tolerable.
Hide
Alex Miller added a comment -

-7 patch adds print support and tests for float infinity and nan.

Show
Alex Miller added a comment - -7 patch adds print support and tests for float infinity and nan.
Hide
Alex Miller added a comment -

-8 patch makes the double symbol reader tests more precise by checking value, not just type

Show
Alex Miller added a comment - -8 patch makes the double symbol reader tests more precise by checking value, not just type

People

Vote (7)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: