@spamegg1

Thank you for submitting this PR.

The "lint" failure can be corrected by running scalafmt at the command line. At the project root "scripts/scalafmt",
then git add, git commit, git push. Yell, if that does not work for you. I usually wait for the rest of CI to finish
to see if there are other errors which can be corrected.

Doc built sucessfully, so that is a good thing.

Almost there, the failures are becausePlatform.hasCompliantNullPointers is not defined. But then
we knew that.

The third alternative in one of my review comments suggests defininghasCompliantNullPointers
with appropriate comments, at the top of the file and not mucking around with Platform.

Looks good to me (LGTM). Well done@spamegg1

I am going offline now, but will check again this evening to see if CI encountered any problems.

Just another monkey wrench but you should only use javalibs not Scala but you can useScalaOps to get some Scala like operations.https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/util/ScalaOps.scala

@ekrich

you should only use javalibs not Scala but you can useScalaOps to get some Scala like operations.

I have an implementation that passes the tests. What can I use in place ofscala.collection.immutable.List[CharSequence] that still follows along Wojciech's approach above, and good performance-wise? (Lots of stuff in thejava/util folder but I'm not too familiar with them)

FYI, the reason for the "please do not use scalalib" is to break a dependency where scalalib relies upon
javalib which relies upon scalalib. We are trying to eventually get rid of all scalalib usage in the javalib
code. If you are reading existing code, you will see many such, mostly from the early days of Scala Native.
The current practice is to be stricter with new code being added or undergoing major renovation.

For me, most of the time knowing the reasoning for a prohibition makes it easier to live with it.

@ekrich Please keep me on the rails here...
I believe the "rule of thumb" is that Scala Collections are not used injavalib but can be used in
the Tests for javalib, i.e.testsuite. I usually avoid the latter just to avoid confusion in my head.

I also usually use a simpleList and just avoid any Scala specific methods on List. That is use
only the methods defined in JVM. One can be a little more explicit and specifyjava.util.List.
Sometimes you will see imports at the top of the fileimport java.{util => ju} which allow
the former to becomeju.List.

What pattern matching is concerning you. I did a quick look through theStringJoiner.scala file
and nothing jumped out as "non-JVM". Sorry if I missed obvious things in my haste.

@LeeTibbert

I also usually use a simpleList and just avoid any Scala specific methods on List. That is use only the methods defined in JVM. One can be a little more explicit and specifyjava.util.List.

Considering Wojciech's approach above (relying on prepend:: being cheap), how can I usejava.util.List to achieve something similar?

I did a quick look through the StringJoiner.scala file and nothing jumped out as "non-JVM".

My current implementation changed completely, I haven't pushed the changes yet. I'm usingscala.collection.immutable.List[CharSequence].

Should I just push it?

@spamegg1 I really should have given more info like Lee has done. I have been doing this awhile so I wasn't really communicating enough information.ScalaOps gives you basic Scala Operations likeforeach and the like so you can usejava.util.List andjava.util.Set etc. and not have to give up all the niceties of Scala.

We try to learn from Scala.js which has removed all the Scala stdlib from their javalib so we are headed that direction. Most of our collection code has examples because much of it has been ported from Scala.js so that is a good resource many times. Here is some code, it has 2 annotations from the Scala stdlib but not much else.

https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/util/Properties.scala

This has examples ofScalaOps.exists|foldLeft.https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/util/IdentityHashMap.scala

@spamegg1 CI is all green, great.

I want you/us to succeed early, especially on a first PR.

Without holding up this PR, let me mention a few things for one or more possible PRs.
I mention these as encouragement and to illustrate some of the factors that go into
SN library code. Also the power of supportive review to improve the product (but not
at the last minute).

I have heard the expression "Those who propose, dispose." more than
once. (Meaning, "You think it is a good idea, you do it." Said to keep projects from
ballooning with 1+s and never getting shipped).

• While reviewing the later change to this code, I discovered that twoString.join() are not
  implemented. Carve another notch on your coup stick.
  I created PRjavalib String is missing two static join methods #3408 to keep track of the issue.

• InStringJoiner.scala line 33, it seems likeprivate var isEmpty: Boolean = true and its
  associated bookkeeping later can be replaced bycontents.isEmpty() intoString() andmerge().
  This gets rid of avar and bookeeping. Taking the size of an ArrayList is more expensive than
  accessing a variable, but it is pretty cheap and easy to get right. I suspect the crossover point
  where one method call is cheaper than a series of increments (in adds) is probably about 10 or so.
  I have no data to backup that WAG (guess).

• The implementation ofStringJoiner.length looks easy to get right but expensive. Instantiating the
  String and doing nothing beyond taking its length means doing a, potentially, lot of memory allocations.

  Seems like a follow on could determine the length by doing the math. All the lengths are known at the
  time of the call. Something like (untried):
  lengthOfPrefix + lengthOfSuffix + (lengthOfSeparator * lengthOfArrayList) +
  contents.foldLeft(0)((a: Int, b: String) => a + b.length())

  The potentially painful part is that Scala 2 can have problems with anonymous_functions/lambdas,
  especially with certain version of Java. Scala2.12 & Java 17 has been giving me a kicking several times
  this week.

  I mention this to cut down frustration if a first submission to CI fails.

  Wojciech has fixed myStreamTest.scala several (three or more, set face red) times., so the
  iterate() tests there give an example of making a lambda robust. I usually just create a
  localdef for the function and use that. (I did such two days or so ago. If you want, I can
  try to find the file.)

@spamegg1
The timing of this PR is excellent, see Issue#3409. It sure has flushed out a number of bugs.
Thank you

Bug reports are a sign that someone is actually using the product of your hard work (I keep telling myself that
when I get bug reports ;-)).

I tried using the three argument constructor ofStringJoner in this PR fromjava.util.streamCollectors.scala.
I got compilation errors, which sent me off to the races of reading both the Scala.js code and this Scala Native code.
I was quite astonished by the compilation errors, sinceStreamJoinerTest has cases which use the 3Arg form.

1. Both have a@inline annotation. That may be good for Scala.js. I can understand its "this was supposed to be
   an easy, straightforward port' transfer to SN. I suspect/believe that the@inline should be deleted. My concern
   is that it will generate lots of code at each use site. (and I am using it in private work).

2. The problem at hand. The current code declares the StringJoiner constructor with private args.

final class StringJoiner  private (                                                       delimiter: CharSequence,                                                        prefix: CharSequence,                                                           suffix: CharSequence                                                        ) extends AnyRef {

I believe this makes the constructor inaccessible (outside the package, including sub-packages.??)

Scala.Js declares the primary constructor as private but then has a 1-arg & 3-arg public method (corresponding
to Java API (edited):

final class StringJoiner private (delimiter: String, prefix: String, suffix: String) extends AnyRef {  def this(delimiter: CharSequence) =    this(delimiter.toString(), "", "")  def this(delimiter: CharSequence, prefix: CharSequence, suffix: CharSequence) =    this(delimiter.toString(), prefix.toString(), suffix.toString())

Ithink that thetoString() will trigger the NullPointerExceptions described in the API without requiring
explicit Objects.NotNull() checks.

There must be a good reason why the code of this PR is the way it is. Sorry if I am missing it.

PS: Once I hacked a private copy ofStringJoiner to get the 3-arg constructor, the rest, including merge(),
worked just fine in my work-in-progressCollectors.joiner() tests.

There must be a good reason why the code of this PR is the way it is. Sorry if I am missing it.

It's my mistake. I wasn't sure but I thought switching toCharSequence made the alternate constructor redundant since everyString is aCharSequence. Didn't pay attention to theprivate keyword there! (I've never seen it used that way in Scala code before.) Yes the null checks were added due to lack oftoString().

I'm not sure how to make an alternate constructor now. Should it also takeCharSequences? Then I getambiguous reference to overloaded definition. Do I change it to takeString instead? But above Wojciech said to use Scala.js's alternate constructor (withCharSequences) as the main one... should I remove theprivate from the main constructor instead?

So confused 😕

@spamegg1 I need to apologize. You may be confused because it looks like I gave you advise which
conflicts with theCharSequence advice given above by@WojciechMazur. In re-reading this series of
replys, I just came across that. Sorry.

I think the part that is tripping me/us up in trying to convert the original .Js code to SN is the
convenient but limiting Scalamkstring(). That requires String arguments, and the pressure to
use Strings backs up from there all the way to the constructor.

I have spent several hours on this in the middle of the night and have run out of time.
I will be offline most of the time until Thursday.

I created a private study version of StringJoiner.scala. That revealed a problem with having
the primary constructor arguments as CharSequence. As thetestState test in StringJoinerTest
shows, those arguments can be changed from outside after they are passed to the StringJoiner
argument. That means that StringJoiner needs to make a deep copy. Taking a deep copy of aStringBuilder
or aCharSequence other thanString is not going to be simple. Cost/benefit considers may well drive
us to usingString as the simplest copy.

I suggest that this PR, assuming it passes CI, be merged and remaining issues be addressed in one or
more followup PRs.

I will check in on Thursday to see where the discussion goes.

@LeeTibbert
Yes in my previous approach I was making immutableString copies of the parameters to fixtestState.

In the latest commit I changed the primary (private) constructor to useString instead, while the auxiliary (public) constructor still usesCharSequence. SotestState is fine now.

This PR gives a good outward facing base level¹. The reasonably done is far better than the absent
prefect.

I have a next evolution PR ready to submit after this PR is merged. It uses aStringBuilder and some other
details to reduce the number of String allocation and character copies inStringJoiner.scala. It passes
StringJoinerTest of this PR, without changing the tests.

It also passesCollectorsTest.scala using my WIPCollectors#joining code which usesStringJoiner.

I have not done performance metrics but think it will be correct and have a lower resource cost.

This will make a difference when someone wants to Collect a stream of 10_000 elements.

I think submitting the file as a follow-on PR will cause less confusion than posting a gist.
Let's get a firm baseline established.

@spamegg1

Congratulation on the merge of your first Scala Native PR.

Programming/Software_Engineering in the large.

Looking forward to your next PR.