Skip to content

[Mono] Fix all remaining C4018 warnings and enable the check during build - #71269

Merged
fanyang-mono merged 22 commits into
dotnet:mainfrom
fanyang-mono:fix_warnings_3
Jul 7, 2022
Merged

fanyang-mono merged 22 commits into
dotnet:mainfrom
fanyang-mono:fix_warnings_3

Conversation

@fanyang-mono

Copy link
Copy Markdown
Member

Fix remaining C4018 warnings on Mono runtime x86/x64 Windows build, aligning with SDL requirements (#66154).

@fanyang-mono

Copy link
Copy Markdown
Member Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/mono/mono/mini/decompose.c Outdated
Comment thread src/mono/mono/mini/decompose.c Outdated

@lambdageek lambdageek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

couple of nits but mostly LGTM.

I do think that business with the membase_offset instructions needs to be addressed in another way. the offset should be signed.

Comment thread src/mono/mono/mini/interp/interp.c
Comment thread src/mono/mono/mini/interp/transform.c Outdated
Comment thread src/mono/mono/mini/mini-generic-sharing.c Outdated
Comment thread src/mono/mono/mini/mini-generic-sharing.c Outdated
Comment thread src/mono/mono/mini/mini-generic-sharing.c Outdated
fanyang-mono and others added 4 commits July 5, 2022 11:21

@lateralusX lateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looked through all except:

aot-compiler.c and mini-llvm.c.

There are several occasions where we switched from int to uint but kept use of %d in formatting masks using the variable. I have not flagged them since they will still be ok according to size, but formatting string will still interpret the value as a signed int, I guess that is still ok, so I have not made any additional comments on that pattern.

Thanks for working through and fix the remaining C4018, great work!

Comment thread src/mono/cmake/config.h.in
Comment thread src/mono/mono/component/debugger-agent.c Outdated
Comment thread src/mono/mono/eglib/giconv.c Outdated
Comment thread src/mono/mono/eglib/giconv.c Outdated
Comment thread src/mono/mono/metadata/class-setup-vtable.c Outdated
Comment thread src/mono/mono/mini/mini-exceptions.c Outdated
Comment thread src/mono/mono/profiler/helper.c Outdated
Comment thread src/mono/mono/sgen/sgen-los.c Outdated
Comment thread src/mono/mono/utils/mono-threads.c
Comment thread src/mono/mono/metadata/class-internals.h
@fanyang-mono

Copy link
Copy Markdown
Member Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono

Copy link
Copy Markdown
Member Author

Build Android x86 Release AllSubsets_Mono failed on rolling build as well
Build tvOS arm64 Release AllSubsets_Mono timed out on other PR runs as well.

@fanyang-mono
fanyang-mono merged commit 5529dc7 into dotnet:main Jul 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
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.

4 participants