Skip to content

Add insert methods to java.lang.StringBuilder #1106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 25, 2014
Merged

Add insert methods to java.lang.StringBuilder #1106

merged 1 commit into from
Sep 25, 2014

Conversation

l15k4
Copy link

@l15k4 l15k4 commented Sep 24, 2014

StringBuilder is missing insert methods, I tried to make it as similar as possible to append methods.

Two things:

  1. inserting is using substring instead of splitAt cause it's obviously more effective way if you look at the impl
  2. the removing stuff in StringBufferTest is just renaming of a variable that had a misleading name newBuf - it's a builder, not buffer

@scala-jenkins
Copy link

Can one of the admins verify this patch?

@gzm0
Copy link
Contributor

gzm0 commented Sep 24, 2014

Ok to test

@gzm0
Copy link
Contributor

gzm0 commented Sep 24, 2014

Test this please

@sjrd
Copy link
Member

sjrd commented Sep 24, 2014

test this please

@sjrd
Copy link
Member

sjrd commented Sep 24, 2014

@gzm0 commands must be all-lowercase

@scala-jenkins
Copy link

Test FAILed.
Refer to this link for build results: https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/570/

sjrd
sjrd reviewed Sep 25, 2014
View reviewed changes
test-suite/src/test/scala/scala/scalajs/testsuite/javalib/StringBufferTest.scala
@@ -12,6 +12,15 @@ import org.scalajs.jasminetest.JasmineTest

object StringBufferTest extends JasmineTest {

def shouldThrow[T : Manifest](fn: => Unit) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use T : ClassTag instead. Manifest is deprecated since 2.10, and does not completely work in Scala.js. This is the reason for the failures in our Continuous Integration.

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

Could you also add the same set of methods to java.lang.StringBuffer, please? They're supposed to be kept in sync.

@l15k4
Copy link
Author

l15k4 commented Sep 25, 2014

Ok, I'll make another commit with Manifest fix + StringBuffer inserts

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

I'll make another commit

Amend your commit instead.

@l15k4
Copy link
Author

l15k4 commented Sep 25, 2014

Btw if I do :

testOnly scala.scalajs.testsuite.javalib.StringBufferTest

the test compiles against real java JDK, not the ScalaJS JDK

For instance if you try to do

val b = new java.lang.StringBuffer with SomeTrait

You get :

illegal inheritance from final class StringBuffer

but java.lang.StringBuffer is final only in java JDK, not ScalaJS JDK

Isn't that a little weird ?

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

That's how it's "supposed" to be. Scala.js projects are always compiled against the JVM version of the Java JDK and Scala standard library. That is because scalac expects some Java classes to really be Java classes in order not to crash.

That is why we have to be always very careful to reproduce exactly the binary signature as the one of the Java SDK.

The bug is however that our StringBuffer should be final, to mimic the Java SDK.

@l15k4
Copy link
Author

l15k4 commented Sep 25, 2014

Ok, I'm glad to know this. Should I make a new PR with the commit ammend ?
l15k4@28fc5cf

I'll leave the final thing up to you, it should have its own issue if somebody depended on its non-finality

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

Should I make a new PR with the commit ammend ?

No, you should force-push the branch from which you created the PR (which makes me realize you created the PR from master, which is a bad thing to do). Force-pushing will update the PR.

@l15k4
Copy link
Author

l15k4 commented Sep 25, 2014

I force-pushed it, for next PR I'll make a feature branch

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

LGTM

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

retest this please

@scala-jenkins
Copy link

Test FAILed.
Refer to this link for build results: https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/573/

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

retest this please

@gzm0
Copy link
Contributor

gzm0 commented Sep 25, 2014

We need to add jsZip to the internet...

@scala-jenkins
Copy link

Test FAILed.
Refer to this link for build results: https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/575/

@sjrd
Copy link
Member

sjrd commented Sep 25, 2014

retest this please

@scala-jenkins
Copy link

Test PASSed.
Refer to this link for build results: https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/577/

sjrd added a commit that referenced this pull request Sep 25, 2014
@sjrd
Merge pull request #1106 from l15k4/master
e31f230 
Add insert methods to java.lang.StringBuilder
@sjrd sjrd merged commit e31f230 into scala-js:master Sep 25, 2014
@sjrd
Copy link
Member

sjrd commented Oct 29, 2014

@l15k4 Was this also inspired by an existing JDK's code? If yes, we have to revert this :-(

Btw, could you also please sign the CLA (see #1139). I also see that your commits are under the name @viagraphs. You should configure your GitHub account so it knows that the appropriate e-mail address is attached to your account. Otherwise we can't track the CLA status.
Thanks.

@scala-jenkins
Copy link

Can one of the admins verify this patch?

@sjrd sjrd mentioned this pull request Oct 29, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@l15k4 @scala-jenkins @gzm0 @sjrd