Fix #1796: Nativelib toCString() & fromCString() now return nulls. #1945

LeeTibbert · 2020-10-13T15:18:05Z

In the sweat of thy face shalt thou eat bread, till thou return unto
the ground; for out of it wast thou taken: for null thou art, and
unto null shalt thou return. [1]

This PR was motivated by Issue fromCString(null) returns java.lang.RuntimeException not NullPointerException #1796. There is extensive discussion
of design alternatives in that Issue. This PR implements the
final decision; toCString() & fromCString are to
return null when given a null.
Two test cases for exercising null handling were added to
were added to CStringSuite.scala.

To isolate the changes of this PR, I left CStringSuite.scala
as the old-style Scala Native test Suite it was before I touched
it this time. If/when the core changes of this PR are found
acceptable, I can sweep through and convert that file to JUnit,
under the guidance of: You touch it, you own it.

Documentation:

The standard changelog entry is requested.
I added a one sentence paragraph to docs/user/interop.rst
to make the newly defined behavior accessible to users of Scala Native.

interop.rst had a number (7 or 8) warnings before I touched it.
I left it with the same warnings. That is, I did not introduce
any warning, but, to keep this PR reviewable on-sight, I did
not fix any. I think I have a PR in the queue which fixes some
of those warnings and others; yielding, at that time, a clean build.

Testing:

Safety -

Built and tested ("test-all") in debug mode using sbt 1.3.13 on
X86_64 only . All tests pass.

Efficacy -

The two newly cases in CStringSuite.scala to exercise the changes
of this both passed.

Documentation -

I did a successful manual html build of the doc directory.

[1]: Adapted from original

sjrd

Thanks for the PR. There are several asInstanceOfs that are not necessary, I believe.

nativelib/src/main/scala/scala/scalanative/unsafe/package.scala

unit-tests/src/test/scala/scala/scalanative/unsafe/CStringSuite.scala

LeeTibbert · 2020-10-13T19:55:13Z

@david-bouyssie, @jokade Does this PR provide what you were describing & needing to ease your work?
Did I capture the idea?

``` In the sweat of thy face shalt thou eat bread, till thou return unto the ground; for out of it wast thou taken: for null thou art, and unto null shalt thou return." ``` [^1] * This PR was motivated by Issue scala-native#1796. There is extensive discussion of design alternatives in that Issue. This PR implements the final decision; toCString() & fromCString are to return null when given a null. * Two test cases for exercising null handling were added to were added to CStringSuite.scala. To isolate the changes of this PR, I left CStringSuite.scala as the old-style Scala Native test Suite it was before I touched it this time. If/when the core changes of this PR are found acceptable, I can sweep through and convert that file to JUnit, under the guidance of: You touch it, you own it. Documentation: * The standard changelog entry is requested. * I added a one sentence paragraph to `docs/user/interop.rst` to make the newly defined behavior accessible to users of Scala Native. `interop.rst` had a number (7 or 8) warnings before I touched it. I left it with the same warnings. That is, I did not introduce any warning, but, to keep this PR reviewable on-sight, I did not fix any. I think I have a PR in the queue which fixes some of those warnings and others; yielding, at that time, a clean build. Testing: Safety - + Built and tested ("test-all") in debug mode using sbt 1.3.13 on X86_64 only . All tests pass. Efficacy - + The two newly cases in CStringSuite.scala to exercise the changes of this both passed. Documentation - + I did a successful manual html build of the `doc` directory. [^1]: Modified from [original](https://www.phrases.org.uk/meanings/ashes-to-ashes.html) Travis CI restarts: 1) All but one runs completed green. The one which failed did so with a Coursier fetch error, apparently unrelated to this PR. Restarting.

ekrich

LGTM now.

Travis CI restart #2. Failed Travis job (scalafmt, but not in scalafmt) unrelated to the PR, just after "store build cache". Travis CI restart scala-native#3. Two failed jobs unrelated to this PR. One was a failed CLA check. Second was a failed Coursier fetch.

* PR scala-native#1945 added two cases to unsafe.CStringSuite.scala. To aid review of scala-native#1945, I did not convert CStringSuite from the classical Scala Native test format to JUnit. Rather, I left an IOU in the commit message for scala-native#1945. This PR pays off that IOU. * There should be no functional changes. A trial testOnly of CSuiteTest.scala reveals no changes beyond changed test names and JUnit style output. * It looks as though the CStringSuite tests had been written at different times, with slightly different assert styles. I am supposing that the later authors did not want to alter existing, working tests, again to minimize and isolate changes. I changed some of the asserts to use a consistent "expected, result" format. This is the convention documented by JUnit and some/most of the existing tests. Future readers will expect that a JUnit test follow the JUnit convention. Documentation: * As a number of tests are changing to JUnit the Release Notes will need a blanket statement saying "Many tests changed to JUnit.". This test would be covered by that statement. Testing: Safety - + Built and tested ("test-all") in debug mode using sbt 1.3.13 on X86_64 only . All tests pass. Efficacy - + JUnit style output is evident upon execution.

* PR scala-native#1945 added two cases to unsafe.CStringSuite.scala. To aid review of scala-native#1945, I did not convert CStringSuite from the classical Scala Native test format to JUnit. Rather, I left an IOU in the commit message for scala-native#1945. This PR pays off that IOU. * There should be no functional changes. A trial testOnly of CSuiteTest.scala reveals no changes beyond changed test names and JUnit style output. * It looks as though the CStringSuite tests had been written at different times, with slightly different assert styles. I am supposing that the later authors did not want to alter existing, working tests, again to minimize and isolate changes. I changed some of the asserts to use a consistent "expected, result" format. This is the convention documented by JUnit and some/most of the existing tests. Future readers will expect that a JUnit test follow the JUnit convention. Documentation: * As a number of tests are changing to JUnit the Release Notes will need a blanket statement saying "Many tests changed to JUnit.". This test would be covered by that statement. Testing: Safety - + Built and tested ("test-all") in debug mode using sbt 1.3.13 on X86_64 only . All tests pass. Efficacy - + JUnit style output is evident upon execution. Travis CI restarts: 1) Failures unrelated the changes of this PR: One CLA failure, two Coursier. Looks like Travis's network is having a bad hair day. All the other cases are successful/green.

…rn nulls. (scala-native#1945)

…now return nulls. (scala-native#1945)" This reverts commit 978eb96.

…rn nulls. (scala-native#1945)

sjrd requested changes Oct 13, 2020

View reviewed changes

sjrd approved these changes Oct 13, 2020

View reviewed changes

LeeTibbert mentioned this pull request Oct 13, 2020

Fix #1796: fromCString(null) now returns proper NullPointerException. #1831

Closed

LeeTibbert mentioned this pull request Oct 13, 2020

fromCString(null) returns java.lang.RuntimeException not NullPointerException #1796

Closed

LeeTibbert requested a review from ekrich October 13, 2020 19:59

LeeTibbert force-pushed the PR_nativelib_CStringNulls_I1796_2020-10-13T0942-0400 branch from 957151c to 1a11b7a Compare October 13, 2020 20:02

ekrich approved these changes Oct 13, 2020

View reviewed changes

LeeTibbert force-pushed the PR_nativelib_CStringNulls_I1796_2020-10-13T0942-0400 branch from 370c749 to bb7e5fa Compare October 13, 2020 23:46

LeeTibbert force-pushed the PR_nativelib_CStringNulls_I1796_2020-10-13T0942-0400 branch from bb7e5fa to 6a8b3b7 Compare October 14, 2020 04:38

sjrd changed the title ~~Fix 1796: Nativelib toCString() & fromCString() now return nulls.~~ Fix #1796: Nativelib toCString() & fromCString() now return nulls. Oct 14, 2020

sjrd merged commit 064d434 into scala-native:master Oct 14, 2020
1 check passed

LeeTibbert mentioned this pull request Oct 14, 2020

WIP: Convert unsafe.CStringSuite.scala to JUnit unsafe.CStringTest.scala #1947

Closed

vicopem pushed a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Fix scala-native#1796: Nativelib toCString() & fromCString() now retu…

978eb96

…rn nulls. (scala-native#1945)

vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Revert "Fix scala-native#1796: Nativelib toCString() & fromCString() …

14432ea

…now return nulls. (scala-native#1945)" This reverts commit 978eb96.

vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Revert "Fix scala-native#1796: Nativelib toCString() & fromCString() …

3a369a6

…now return nulls. (scala-native#1945)" This reverts commit 978eb96.

ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021

Fix scala-native#1796: Nativelib toCString() & fromCString() now retu…

d2f10a6

…rn nulls. (scala-native#1945)

WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021

Fix scala-native#1796: Nativelib toCString() & fromCString() now retu…

d77957e

…rn nulls. (scala-native#1945)

Fix #1796: Nativelib toCString() & fromCString() now return nulls. #1945

Fix #1796: Nativelib toCString() & fromCString() now return nulls. #1945

LeeTibbert commented Oct 13, 2020

sjrd left a comment

LeeTibbert commented Oct 13, 2020

ekrich left a comment

Fix #1796: Nativelib toCString() & fromCString() now return nulls. #1945

Fix #1796: Nativelib toCString() & fromCString() now return nulls. #1945

Conversation

LeeTibbert commented Oct 13, 2020

sjrd left a comment

Choose a reason for hiding this comment

LeeTibbert commented Oct 13, 2020

ekrich left a comment

Choose a reason for hiding this comment