Fork me on GitHub
#clojure-dev
<
2015-07-08
>
clojuregeek17:07:28

i have a patch candidate for CLJ-1449 http://dev.clojure.org/jira/browse/CLJ-1449 if anyone has some feedback for me simple_smile

ghadi17:07:26

quick glance, the interop calls (indexOf, etc) aren't defined on CharSequence ,but on String

ghadi17:07:40

I think that's an oversight in the requirements in the description

clojuregeek17:07:33

ok reading https://github.com/clojure/clojure/blob/master/src/clj/clojure/string.clj#L33 .. i understood to use CharSequence but it possible i don’t fully understand simple_smile

ghadi17:07:37

clojuregeek: it seems that a lot of the functions in clojure.string cast from CharSequence -> String through .toString

clojuregeek18:07:39

for starts-with? for example, it takes a string, value and returns boolean .. i looked at how blank? was implemented .. takes a CharSequence.. returns a boolean

ghadi18:07:23

there is a difference though, blank? only calls .charAt and length, both of which are defined on CharSeq

ghadi18:07:45

so it doesn't need to cast to String

clojuregeek18:07:23

so all the new functions need to use ^String not ^CharSequence ?

ghadi18:07:21

they should cast inside if that's what the old ones do

clojuregeek18:07:53

these are new methods to be added to clojure.string

ghadi18:07:40

understood, they should cast like the existing ones do

ghadi18:07:58

take a CharSequence, cast to String, operate on string

clojuregeek18:07:24

where in blank? is it casting s as String ?

ghadi18:07:59

it doesn't cast in blank? because blank? happens to have everything necessary on CharSequence

ghadi18:07:37

blank? is not a good example, see the other calls to (.toString s)

clojuregeek18:07:26

ok i see triml casts to a string

clojuregeek18:07:22

so in index-of i need to cast it to a string before i can call indexOf ?

ghadi18:07:07

yup because indexOf only exists on String which is a subclass of CharSequence

alexmiller18:07:00

just want to say +1 to ghadi's comments

ghadi18:07:09

another approach would be to reimplement indexOf in clojure, but I think that might be poopooed upon

alexmiller18:07:12

also, you have added "1.9" - should be 1.8

alexmiller18:07:30

ghadi: yeah, we should just call java here

clojuregeek18:07:30

ok I didn’t know how long it would take to get accepted simple_smile

alexmiller18:07:39

well let's assume 1.8 :)

ghadi18:07:53

laugh and cry on that comment

alexmiller18:07:00

you can test with StringBuffer/StringBuilder instances for something that is a CharSequence but not a String

clojuregeek18:07:39

is this correct? when you pass a char to index of?

332 │    (if (char? value)
 333 │     (.lastIndexOf s (int value))
 334 │     (.lastIndexOf s value)))

alexmiller18:07:21

that should work (although I'd verify by looking at the byte code)

alexmiller18:07:31

you could also do (.lastIndexOf s ^char value)

clojuregeek18:07:22

ok how would i look at the byte code?

alexmiller18:07:44

"mvn clean compile" to build clojure

alexmiller18:07:28

then: javap -c -classpath target/classes clojure.string\$index_of

alexmiller18:07:47

you might want to pipe that into an editor

alexmiller18:07:05

then check out the invoke methods

alexmiller18:07:54

I'm happy to get on skype and walk through it with you or something

alexmiller18:07:20

my book went to production today and I haven't slept much this week - sorry for not responding to your email :)

clojuregeek18:07:58

oh its ok!! no problem!!

alexmiller18:07:40

also, try adding (while doing dev): (set! warn-on-reflection true) to the top of that ns - you'll see it's generating a bunch of warnings from your changes right now

alexmiller18:07:03

those should have been around warn-on-reflection, not bold text :)

clojuregeek18:07:29

ok i see the warnings. I was going to say.. it ‘appeared to work’ on CharSequences so I was puzzled

alexmiller18:07:50

well stringbuffer for example cleverly implements the same methods as string for things like indexOf, but CharSequence does not define it

alexmiller18:07:33

clojure will generate the proper invocation and it will succeed in both cases - jvm ftw :)

andrewhr18:07:06

I would add that smaller arity impl could just call greater a arity ones, to avoid duplication of code (since they are essentially the same). Not sure how much overhead this could impact

alexmiller18:07:28

clojuregeek: actually without hints, it's just going to make a reflective call on the instance, so you don't want that

alexmiller18:07:50

in the byte code you can see "Method clojure/lang/Reflector.invokeInstanceMethod" - you don't want to see that :)

clojuregeek18:07:43

this?

[java] Reflection warning, clojure/string.clj:324:6 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown).
     [java] Reflection warning, clojure/string.clj:327:5 - call to method indexOf on java.lang.String can't be resolved (argument types: int, unknown).
     [java] Reflection warning, clojure/string.clj:328:5 - call to method indexOf on java.lang.String can't be resolved (argument types: unknown, unknown).

alexmiller18:07:21

yeah, that's the warning - I was referring to the bytecode on the last one, but saying the same thing either way

alexmiller18:07:55

andrewhr: my guess is that the fastest path is to get the right args to the right java.lang.String method as directly as possible and then let the jit do its thing.

alexmiller18:07:16

that is, I am more concerned about performance than duplication in this case

andrewhr18:07:45

makes sense alexmiller, good point

andrewhr18:07:09

clojuregeek: reading java docs I learned that indexOf doesn't care about fromIndex bounds: they could be negative or greater than actual string length. What do you think about expressing this behavior on the tests themselves? Worth the trouble?

clojuregeek18:07:34

part of my thinking on the tests was, the decisions i do (if char?) should be fully tests .. but testing the output of the javaop doesn’t need to be tested here, those tests should be done on java side .. so i held back some on my test obsessiveness : )

alexmiller18:07:07

I think I'd say it does care about it but has well-defined semantics when it is <0 or >length. seems like those should be covered in the tests.

alexmiller18:07:27

you don't need to verify the output of javap

alexmiller19:07:07

but I use it to ensure the compiled version really is doing what I expect in a method like this in the std lib that I would expect to get called a lot

alexmiller19:07:37

it might be advisable to leave the (set! ) call in the patch so warnings would show up - we do that in many other places in core

clojuregeek19:07:41

ok i’ll leave the set in the patch

alexmiller19:07:37

it would be fun to think about test.check properties for these too

alexmiller19:07:13

start with a string, generate another arbitrary string including the first string, then verify that the original string can be found

clojuregeek19:07:20

i would love to do those kinds of tests…

alexmiller19:07:40

test.check is a dependency and we are trying to write more of them

clojuregeek19:07:18

simple_smile ok i am not understanding this type hinting, so if you have time to do a call i think that would help me simple_smile

ghadi19:07:08

clojuregeek: I'd be glad to help sometime.

ghadi19:07:32

will DM you to find a good off-hours time.

clojuregeek21:07:05

would this be the way I would need to refer to my locally compiled version of clojure? https://github.com/technomancy/leiningen/blob/stable/doc/TUTORIAL.md#checkout-dependencies … just want to make sure I don’t spend hours going down a molehill simple_smile

ghadi21:07:17

clojuregeek: you have a couple options here. Simplest: mvn clean package then look in the target/ directory

ghadi21:07:40

java -jar target/clojure-1.8.0-SNAPSHOT.jar will start a bare bare bare repl

ghadi21:07:03

Not nREPL or anything fancy. But it will at least allow you to test quickly

clojuregeek21:07:46

ok, and if I wanted to have that as a dependency in a new clojure project?

ghadi21:07:37

change the version number in pom.xml, then mvn clean install, then refer to that version from a separate project

clojuregeek21:07:30

ok cool, thanks. I wanted to do some benchmarking with criterium

clojuregeek22:07:39

@ghadi: I get this error:

java.lang.IllegalArgumentException: Bad artifact coordinates org.clojure:clojure:jar:clojure 1.8.1, expected format is <groupId>:<artifactId>[:<extension>[:<classifier>]]:<version>
 at org.sonatype.aether.util.artifact.DefaultArtifact.<init> (DefaultArtifact.java:73)
    org.sonatype.aether.util.artifact.DefaultArtifact.<init> (DefaultArtifact.java:56)
` I have this in pom.xml and build, install
<version>1.8.1-master-SNAPSHOT</version>

clojuregeek22:07:55

i see the jar file in my m2 directory

ghadi22:07:43

what do you have in your project dependencies?

clojuregeek22:07:11

i figured it out, imadork

clojuregeek22:07:25

the name didn’t match exactly simple_smile

clojuregeek22:07:00

thanks for your help simple_smile i’m gonna blog about it so i can find it again when I google simple_smile

ghadi22:07:20

#!/bin/bash

M2=$HOME/.m2/repository
CLASSPATH=${M2}/criterium/criterium/0.4.3/criterium-0.4.3.jar:src
JOPTS='-Xmx4G -XX:+UseG1GC'
CLOJURE_JAR=$M2/org/clojure/clojure/${CLOJURE_VERSION}/clojure-${CLOJURE_VERSION}.jar
java $JOPTS -cp ${CLASSPATH}:${CLOJURE_JAR} clojure.main

ghadi22:07:36

Another option for criterium + clojure repl ^

ghadi22:07:39

without lein

ghadi22:07:51

Just save that as script.sh and do CLOJURE_VERSION=1.7.0 script.sh

ghadi22:07:12

clojure starts pretty fast when lein isn't in the picture