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

DateTime and TimeSpan are not handled as primitive types anymore #2788

Merged

Conversation

josesimoes
Copy link
Member

@josesimoes josesimoes commented Oct 11, 2023

Description

  • Remove these throughout the code base.
  • Add DateTime::NewObject and rework code accordingly.
  • Update data type lookup for DateTime and TimeSpan so they end up as being treated like value type types.

Motivation and Context

How Has This Been Tested?

  • mscorlib unit tests.
  • GC long run test.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

- Remove these throughout the code base.
- Add DateTime::NewObject and rework code accordingly.
- Update data type lookup for DateTime and TimeSpan so they end up as value type types.
@josesimoes josesimoes force-pushed the remove-dt-ts-from-primitive-types branch from 253c7fc to 4b07c76 Compare October 11, 2023 14:25
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Those are quite some changes. Is there any impact on any of the existing native code using those elements? I'm thinking of everywhere we are using TimeSpan and DateTime in the parameters of a function for example.

@josesimoes
Copy link
Member Author

Those are quite some changes. Is there any impact on any of the existing native code using those elements? I'm thinking of everywhere we are using TimeSpan and DateTime in the parameters of a function for example.

Everything that was somehow impacted by this change has been taken care of and thoroughly tested.
At the end of the day this is completely transparent to the C# applications.

@Ellerbach
Copy link
Member

My question was on the native side, not on the C# side where, it doesn't matter at all. I fully trust you that all has been tested :-) I just want to get an understanding if anything could be impacted somehow by side effect.

@josesimoes josesimoes added Area: Common libs Everything related with common libraries and removed ⚠️ DO NOT MERGE ⚠️ Status: under review labels Oct 12, 2023
@josesimoes josesimoes merged commit a90a2ab into nanoframework:main Oct 12, 2023
24 checks passed
@josesimoes josesimoes deleted the remove-dt-ts-from-primitive-types branch October 12, 2023 14:53
@josesimoes
Copy link
Member Author

Answering your question above: no impact on the existing code, apart from what was already changed to accommodate these changes.

josesimoes added a commit that referenced this pull request Oct 16, 2023
@josesimoes josesimoes mentioned this pull request Oct 16, 2023
13 tasks
josesimoes added a commit that referenced this pull request Oct 16, 2023
***NO_CI***
TekuSP added a commit to TekuSP/nf-interpreter that referenced this pull request Oct 18, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Common libs Everything related with common libraries Breaking-change Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants