Skip to content

Conversation

@josesimoes
Copy link
Member

@josesimoes josesimoes commented Sep 1, 2025

Description

  • Update mscorlib declaration.

Motivation and Context

How Has This Been Tested?

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/declarations (update dependencies or assembly declarations 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).

Summary by CodeRabbit

  • Chores

    • Synchronized internal mappings and metadata across build configurations to maintain consistent behavior.
    • Bumped assembly data version to reflect internal table realignment; no user-facing changes.
  • Refactor

    • Reindexed internal method tables by inserting placeholders for alignment and parity across build modes.
    • Ensured consistency between configurations without altering runtime logic or public APIs.

@nfbot nfbot added the Type: dependencies Pull requests that update a dependency file(s) or version label Sep 1, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 1, 2025

Walkthrough

Inserted NULL placeholders into corlib method_lookup tables (reindexing entries), updated assembly metadata constants for both NANOCLR_REFLECTION modes, and incremented the g_CLR_AssemblyNative_mscorlib trailing initializer count from 24 to 25.

Changes

Cohort / File(s) Change Summary
Method lookup table adjustments
src/CLR/CorLib/corlib_native.cpp
Inserted NULL placeholders before Library_corlib_native_System_Guid::GenerateNewGuid___STATIC__SZARRAY_U1 and before Library_corlib_native_System_MathInternal::Abs___STATIC__I4__I4 across multiple method_lookup blocks; changes applied identically for NANOCLR_REFLECTION TRUE and FALSE, reindexing subsequent entries.
Assembly metadata updates
src/CLR/CorLib/corlib_native.cpp
Replaced assembly metadata constants: in the TRUE branch 0x1549C8560xC12CAE16; in the FALSE branch 0xF0208DC00xA02068F1.
Data initializer sizing
src/CLR/CorLib/corlib_native.cpp
Updated final g_CLR_AssemblyNative_mscorlib trailing initializer from { 100, 5, 0, 24 } to { 100, 5, 0, 25 } to reflect added NULL entries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Potential review focus:

  • Verify inserted NULL positions preserve intended method index alignments and no off-by-one effects elsewhere.
  • Confirm updated assembly metadata constants match expected build/signature outputs.
  • Ensure the trailing initializer count change aligns with other metadata or generated tables.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Update mscorlib declaration" directly relates to the changeset, which updates mscorlib-related data initializers including method_lookup tables and assembly metadata in corlib_native.cpp. The title accurately summarizes the main change at an appropriate level of abstraction and is neither vague nor misleading. A teammate reviewing PR history would clearly understand this involves updating mscorlib declarations without needing additional context from the summary.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-mscorlib

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94d98ce and 64f8695.

📒 Files selected for processing (1)
  • src/CLR/CorLib/corlib_native.cpp (5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
📚 Learning: 2024-09-25T11:28:38.536Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.

Applied to files:

  • src/CLR/CorLib/corlib_native.cpp
📚 Learning: 2024-10-08T15:52:09.445Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.

Applied to files:

  • src/CLR/CorLib/corlib_native.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: nf-interpreter (Check_Code_Style)
  • GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets MXCHIP_AZ3166)
  • GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
  • GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
  • GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
  • GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
  • GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (4)
src/CLR/CorLib/corlib_native.cpp (4)

573-606: LGTM! Consistent method table expansion.

The NULL placeholders inserted before GenerateNewGuid and Abs are correctly positioned and maintain proper table structure. This pattern suggests reserving slots for methods added in CoreLibrary that will be implemented later.


1311-1341: LGTM! Reflection mode parity maintained.

The NULL insertions in the NANOCLR_REFLECTION == FALSE configuration exactly mirror those in the TRUE configuration, ensuring both code paths remain synchronized.


1521-1521: LGTM! Assembly version/revision updated.

The increment from 24 to 25 in the trailing initializer correctly reflects the assembly revision update. This appears to be a version tuple (likely major.minor.build.revision) rather than a method count.


1510-1514: Manually verify assembly metadata constants match CoreLibrary#247.

The hex constants (0xC12CAE16 and 0xA02068F1) could not be verified programmatically due to API limitations. Since these assembly identity checksums are critical for runtime assembly resolution, confirm they match the corresponding values in nanoframework/CoreLibrary#247 before merging.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@josesimoes
Copy link
Member Author

Waiting for mscorlib release in order to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: dependencies Pull requests that update a dependency file(s) or version ⚠️ DO NOT MERGE ⚠️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants