tools.reader

Reader conditionals

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Reader conditionals

See: http://dev.clojure.org/display/design/Reader+Conditionals

CLJ-1424 is the accompanying patch for Clojure.

Patch: trdr-14-5.patch

  1. tools.reader-feature-expressions.diff
    15/May/14 5:43 PM
    7 kB
    Ghadi Shayban
  2. tools.reader-feature-expressions-2.diff
    04/Aug/14 12:56 PM
    6 kB
    Alex Miller
  3. tools.reader-feature-expressions-3.diff
    05/Aug/14 2:10 PM
    6 kB
    Alex Miller
  4. trdr-14-4.diff
    21/Jan/15 4:34 PM
    15 kB
    Alex Miller
  5. trdr-14-5.patch
    08/Apr/15 1:58 PM
    26 kB
    Alex Miller

Activity

Hide
Nicola Mometto added a comment - - edited

WIP in the reader-conditionals branch https://github.com/clojure/tools.reader/tree/reader-conditionals
The additional commits on top of 7db11b110770d7c3925194a89baa8b8a773bf431 address the concerns reported by Alex in previous comments (except the use of LinkedList) and make tools.reader use the existing ReaderConditional and TaggedLiteral when available, fallbacking to a custom version when not.

This is the same approach tools.reader uses for ExceptionInfo on clojure 1.3.0 and seems to me a better approach than using different types than clojure itself (when they are available) but I'd like some feedback on this (commit 2c3adbe4c4f0fab8087380ff0dbb1ad44c8739e1)

The additional commits also fix a couple of bugs wrt handling of eof

Show
Nicola Mometto added a comment - - edited WIP in the reader-conditionals branch https://github.com/clojure/tools.reader/tree/reader-conditionals The additional commits on top of 7db11b110770d7c3925194a89baa8b8a773bf431 address the concerns reported by Alex in previous comments (except the use of LinkedList) and make tools.reader use the existing ReaderConditional and TaggedLiteral when available, fallbacking to a custom version when not. This is the same approach tools.reader uses for ExceptionInfo on clojure 1.3.0 and seems to me a better approach than using different types than clojure itself (when they are available) but I'd like some feedback on this (commit 2c3adbe4c4f0fab8087380ff0dbb1ad44c8739e1) The additional commits also fix a couple of bugs wrt handling of eof
Hide
Alex Miller added a comment -

From Nicola: return-on-value is always READ_FINISHED - can we hard-code and remove from the signature?

Show
Alex Miller added a comment - From Nicola: return-on-value is always READ_FINISHED - can we hard-code and remove from the signature?
Hide
Alex Miller added a comment -

two things I'm uncomfortable with:
1) use of LinkedList. it matches the Clojure version but relying on that stateful Java class is gross.
2) the READ_EOF and READ_FINISHED definitions - it's probably better to use (Object.) and identical? than what I'm doing now.

Show
Alex Miller added a comment - two things I'm uncomfortable with: 1) use of LinkedList. it matches the Clojure version but relying on that stateful Java class is gross. 2) the READ_EOF and READ_FINISHED definitions - it's probably better to use (Object.) and identical? than what I'm doing now.
Hide
Alex Miller added a comment -

trdr-14-5 patch is a port of the reader conditional patch at CLJ-1424. It was no fun.

Show
Alex Miller added a comment - trdr-14-5 patch is a port of the reader conditional patch at CLJ-1424. It was no fun.
Hide
Alex Miller added a comment -

Attached new patch trdr-14-4.diff that uses an explicit options map with a :features key to a set of features. These changes match the latest changes in clj-1424, which are patterned after a similar implementation in the Clojure edn reader.

Show
Alex Miller added a comment - Attached new patch trdr-14-4.diff that uses an explicit options map with a :features key to a set of features. These changes match the latest changes in clj-1424, which are patterned after a similar implementation in the Clojure edn reader.
Hide
Alex Miller added a comment -

Updated patch to reflect clj and cljs as initial feature names.

Show
Alex Miller added a comment - Updated patch to reflect clj and cljs as initial feature names.
Hide
Ghadi Shayban added a comment -

Yes thank you for not merging. This is WIP and just one approach for feature expressions. There seem to be at least two couple diverging approaches emerging from the various discussion (Brandon Bloom's idea of read-time splicing being the other.) I would definitely not merge until the community and Rich weighs in.

In any case having all Clojure platforms be ready for the change is probably essential. Also backwards compatibility of feature expr code to Clojure 1.6 and below is also not trivial.

1) *suppress-read* probably doesn't need to be exposed
2) *suppress-read* should probably imply *read-eval* false. You wouldn't want read-ctor to load any classes or call any code or constructors

Show
Ghadi Shayban added a comment - Yes thank you for not merging. This is WIP and just one approach for feature expressions. There seem to be at least two couple diverging approaches emerging from the various discussion (Brandon Bloom's idea of read-time splicing being the other.) I would definitely not merge until the community and Rich weighs in. In any case having all Clojure platforms be ready for the change is probably essential. Also backwards compatibility of feature expr code to Clojure 1.6 and below is also not trivial. 1) *suppress-read* probably doesn't need to be exposed 2) *suppress-read* should probably imply *read-eval* false. You wouldn't want read-ctor to load any classes or call any code or constructors
Hide
Nicola Mometto added a comment - - edited

I looked over the patch and it mostly looks good, a few comments:

  • does *suppress-read* need to be exposed?
  • can you add a docstring for *features*?
  • should *suppress-read* affect read-eval?

That said, has it been decided that this is how feature expressions will be implemented in 1.7?
If not, I'm going to wait to merge this in until a decision is taken, this is a major design point and I don't want to diverge from the official clojure take on this even for a single release: people might start using it, especially since t.r is now the default reader for cljs.

Show
Nicola Mometto added a comment - - edited I looked over the patch and it mostly looks good, a few comments:
  • does *suppress-read* need to be exposed?
  • can you add a docstring for *features*?
  • should *suppress-read* affect read-eval?
That said, has it been decided that this is how feature expressions will be implemented in 1.7? If not, I'm going to wait to merge this in until a decision is taken, this is a major design point and I don't want to diverge from the official clojure take on this even for a single release: people might start using it, especially since t.r is now the default reader for cljs.

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: