Skip to content
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.

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

[iOS][non-icu] HybridGlobalization clean up the code #96974

Merged
merged 12 commits into from
Jan 18, 2024

Conversation

mkhamoyan
Copy link
Member

@mkhamoyan mkhamoyan commented Jan 15, 2024

Fixes #96328

Contributes to #80689

@ghost
Copy link

ghost commented Jan 15, 2024

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #96328

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios

Milestone: -

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

LGTM apart from the pal_config.h issue

@@ -658,3 +658,6 @@ if(NOT DISABLE_EXECUTABLES)

install_with_stripped_symbols(mono-sgen TARGETS bin)
endif()
configure_file(
${CLR_SRC_NATIVE_DIR}/libs/Common/pal_config.h.in
${CMAKE_CURRENT_BINARY_DIR}/pal_config.h)
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't mix our configs with pal_config.h.in, if you need any of the HAVE_* defines then you need to include them in src/mono/cmake/configure.cmake

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to use assert_msg from https://github.com/mkhamoyan/runtime/blob/main/src/native/libs/Common/pal_utilities.h#L27 in pal_placeholders.c (instead of defining them again here https://github.com/mkhamoyan/runtime/blob/main/src/native/libs/System.Globalization.Native/pal_placeholders.c#L18-L38) . But when including "pal_utilities.h" header in pal_placeholders.c, it complains that can't find pal_config.h.

Copy link
Member

@akoeplinger akoeplinger Jan 17, 2024

Choose a reason for hiding this comment

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

You can either move it to a new header file in src/native/minipal since that one is supposed to be shared between runtimes.

Or given our future direction of wanting to stop compiling globalization in mono runtime itself maybe it's better to just copy the assert_msg definition into mono since it will get removed later anyway.

Copy link
Member Author

@mkhamoyan mkhamoyan Jan 18, 2024

Choose a reason for hiding this comment

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

Thanks, kept initial way of copying assert_msg definition into mono.

@mkhamoyan
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Member Author

Failures are not related.

@mkhamoyan mkhamoyan merged commit 4f1a138 into dotnet:main Jan 18, 2024
206 of 213 checks passed
@mkhamoyan mkhamoyan deleted the hybrid_code_cleanup branch January 18, 2024 12:21
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS][non-icu] HybridGlobalization rafactor condition checks / clean up the code
2 participants