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

Alex Miller (Clojure team)18: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

Alex Miller (Clojure team)18:07:12

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

Alex Miller (Clojure team)18: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

Alex Miller (Clojure team)18:07:39

well let's assume 1.8 :)

ghadi18:07:53

laugh and cry on that comment

Alex Miller (Clojure team)18: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)))

Alex Miller (Clojure team)18:07:21

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

Alex Miller (Clojure team)18:07:31

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

clojuregeek18:07:22

ok how would i look at the byte code?

Alex Miller (Clojure team)18:07:44

"mvn clean compile" to build clojure

Alex Miller (Clojure team)18:07:28

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

Alex Miller (Clojure team)18:07:47

you might want to pipe that into an editor

Alex Miller (Clojure team)18:07:05

then check out the invoke methods

Alex Miller (Clojure team)18:07:54

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

Alex Miller (Clojure team)18: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!!

Alex Miller (Clojure team)18: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

Alex Miller (Clojure team)18: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

Alex Miller (Clojure team)18:07:50

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

Alex Miller (Clojure team)18: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

Alex Miller (Clojure team)18:07:28

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

Alex Miller (Clojure team)18: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).

Alex Miller (Clojure team)18:07:21

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

Alex Miller (Clojure team)18: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.

Alex Miller (Clojure team)18: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 : )

Alex Miller (Clojure team)18: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.

Alex Miller (Clojure team)18:07:27

you don't need to verify the output of javap

Alex Miller (Clojure team)19: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

Alex Miller (Clojure team)19: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

Alex Miller (Clojure team)19:07:37

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

Alex Miller (Clojure team)19: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…

Alex Miller (Clojure team)19: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