clojure-emacs / logjam

An interactive, nrepl-oriented logging backend
Eclipse Public License 1.0
1 stars 0 forks source link

Add Logjam code #6

Closed r0man closed 1 year ago

r0man commented 1 year ago

This PR adds the code that used to live in cider-nrepl.

r0man commented 1 year ago

Looking into the test failures ...

r0man commented 1 year ago

This seems to be an issue with 1.12.0-master-SNAPSHOT of Clojure:

The form (drop Long/MAX_VALUE []) throws the following exception:

1. Unhandled java.lang.ArithmeticException
   integer overflow

                 Math.java: 1074  java.lang.Math/toIntExact
                   RT.java: 1247  clojure.lang.RT/intCast
                   RT.java: 1219  clojure.lang.RT/intCast
                  core.clj: 2947  clojure.core/drop
                  core.clj: 2926  clojure.core/drop
                      REPL:   29  logjam.event-test/eval11884
                      REPL:   29  logjam.event-test/eval11884
             Compiler.java: 7177  clojure.lang.Compiler/eval
             Compiler.java: 7132  clojure.lang.Compiler/eval
                  core.clj: 3229  clojure.core/eval
                  core.clj: 3225  clojure.core/eval
    interruptible_eval.clj:   87  nrepl.middleware.interruptible-eval/evaluate/fn/fn
                  AFn.java:  152  clojure.lang.AFn/applyToHelper
                  AFn.java:  144  clojure.lang.AFn/applyTo
                  core.clj:  667  clojure.core/apply
                  core.clj: 1990  clojure.core/with-bindings*
                  core.clj: 1990  clojure.core/with-bindings*
               RestFn.java:  428  clojure.lang.RestFn/invoke

Which seems to work fine in older and the current versions of Clojure.

I open a question on Ask Clojure for this: https://ask.clojure.org/index.php/13033/arithmeticexception-on-clojure-1-12-0-master-snapshot

r0man commented 1 year ago

I disabled running the tests on the Clojure master for now, because I expect this to be fixed/changed eventually.

r0man commented 1 year ago

@vemv Can you review this again please?

bbatsov commented 1 year ago

An in-depth review of so much code takes almost as much time as writing it, so there's little point in doing it IMO. The usage docs and the tests look good and I think that's what matters the most to hit the ground running.

We should also mention somewhere that the library is experimental, the APIs are subject to change, etc. But this can be done in subsequent commits.

vemv commented 1 year ago

I eyed this (although less throughly than in the original PR) and it also lgtm!

Those ^long hints should be (long) calls instead:

(let [foo (int bar)] …​) is the correct way to get a primitive local. Do not use ^Integer etc.

https://clojure.org/reference/java_interop#optimization

^long is OK when used as a defn arg:

Functions have limited support for primitive arguments and return type: type hints for long and double (only these) generate primitive-typed overloads. Note that this capability is restricted to functions of arity no greater than 4.

https://clojure.org/reference/java_interop#primitives