[Java.Interop] JniTypeSignature & CultureInfo, empty strings #1002
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: dotnet#335 Context: 920ea64 Commit 920ea64 optimized `JniTypeManager.AssertSimpleReference()` by using `string.IndexOf(char)`. This dovetails with Issue dotnet#335, which wanted to optimize `JniTypeSignature` by removing calls to `string.Contains(string)`. Fix dotnet#335, and update the `JniTypeSignature` constructor to use `string.IndexOf(char)` instead of `string.Contains(string)`, and use string indexers instead of `string.StartsWith()` and `string.EndsWith()`. `Java.Interop.dll` has no remaining usage of `string.StartsWith()` and `string.EndsWith()`. Additionally, update `JniTypeSignature` so that the empty string `""` is *not* treated as a valid type signature. This is arguably a breaking change, but the empty string never made sense, *and* would throw an `IndexOutOfRangeException` with `JniTypeManager.Parse()`.
jonathanpeppers
approved these changes
Jun 30, 2022
Comment on lines
43
to
45
| public JniTypeSignature (string? simpleReference, int arrayRank = 0, bool keyword = false) | ||
| { | ||
| if (simpleReference != null) { |
There was a problem hiding this comment.
Why is simpleReference allowed to be null?
There was a problem hiding this comment.
@jonathanpeppers: it's a struct, and thus always has a default constructor, i.e. new JniTypeSignature() is always "valid" (can't/won't throw), and couldn't be overridden at the time (that may have since changed, but default provides original semantics). Given that, and that new JniTypeSignature() is semantically identical to new JniTypeSignature(null) and new JniTypeSignature(null, 0, false), why should new JniTypeSigature(null) throw?
Perhaps it should throw, but at the time it didn't make sense to me.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #335
Context: 920ea64
Commit 920ea64 optimized
JniTypeManager.AssertSimpleReference()byusing
string.IndexOf(char).This dovetails with Issue #335, which wanted to optimize
JniTypeSignatureby removing calls tostring.Contains(string).Fix #335, and update the
JniTypeSignatureconstructor to usestring.IndexOf(char)instead ofstring.Contains(string), and usestring indexers instead of
string.StartsWith()andstring.EndsWith().Java.Interop.dllhas no remaining usage ofstring.StartsWith()and
string.EndsWith().Additionally, update
JniTypeSignatureso that the empty string""is not treated as a valid type signature. This is arguably a
breaking change, but the empty string never made sense, and would
throw an
IndexOutOfRangeExceptionwithJniTypeManager.Parse().