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.
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Implement collation native functions functions #86895
Implement collation native functions functions #86895
Changes from 25 commits
164bb0a
1e4e9a5
292b915
9d23f7b
1f8b5d5
7b91b6c
c4a2d3c
001c366
0ec8d79
fa7322c
fc2837b
a7ab572
36de1a9
139a6ba
f3da1e5
5c3f172
ab317f7
24094fe
c425d38
a3f44b4
cbaaf80
caebcbe
71b026e
9dda83a
88c6861
460aba0
3d5a195
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't limited to just the diaeresis diacritic mark is it? If it isn't limited to just diaeresis can we change it to diacritics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: diacritic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I'm understanding this.
searchString
U\u0308 and \u00FC
(Ü and ü
)source string
a\u0308\u0308a and \u0075\u0308
(ä̈a and ü
)Is the search string supposed to be a substring in the source string? Was it supposed to be
U\u0308
and\u00FC
should both be interpreted as substrings ofa\u0308\u0308a and (\u0075\u0308)
in that it matches the portion in the parenthesis?I was actually expecting the not covered cases to be something like, source string is a sequence of unicode code points that represent a mixture of precomposed characters and decomposed characters.
i.e. Source string
H\u00EBll\u006F\u0308 World
(Hëllö World
)Search string
\u0065\u0308ll\u00F6
(ëllö
)These would bypass the precomposed vs precomposed comparison because it would be source
string
H\u00EBll\u00F6 World
and search string\u00EBll\u00F6
However, neither the fully precomposed form of the search string
\u00EBll\u00FB
nor the fully decomposed form of the search string\u0065\u0308ll\u006F\u0308
would match the unicode code point sequence in the source string\u00EBll\u006F\u0308
so it would fall through the Form C and Form D fallbacks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Source should be
"Source is \u00DC and \u0075\u0308"
.search string:
U\u0308 and \u00FC
(Ü and ü) source string:Source is \u00DC and \u0075\u0308
(Source is Ü and ü). Will update it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why are we adding
NoInlining
here and onStartsWithNative
but not on the other ones?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did like is done for
ICU
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/Interop/Interop.Collation.cs#L34But not sure why only these 2 functions have
MethodImplOptions.NoInlining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that it is to avoid regressing the hot path of the method by inlining the PInvoke.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is not specific to globalization. It should have neutral namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the unmanaged side is not returning the NSRange type in the latest iteration. It returns its own Range type. In that case, it should not be called NSRange. It should be called Range to match the unmanaged side and it should be in System.Globalization.iOS.cs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using
NSRange
https://developer.apple.com/documentation/foundation/nsrange?language=objc on pal_collation.m,but to make it compiled on https://github.com/dotnet/runtime/blob/main/src/native/libs/System.Globalization.Native/pal_collation.h I needed to add Range type.
Do you mean
Range
struct should be added in System.Globalization.iOS.cs ? I couldn't find that fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant src/libraries/Common/src/Interop/Interop.Collation.OSX.cs