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

Fixed code to reenable some warnings in CoreCLR #33902

Closed
wants to merge 402 commits into from
Closed

Fixed code to reenable some warnings in CoreCLR #33902

wants to merge 402 commits into from

Conversation

ivdiazsa
Copy link
Member

@ivdiazsa ivdiazsa commented Mar 21, 2020

Added some necessary casts and fixed string formats to comply with warnings 4302, 4311, 4312, and 4477 in CoreCLR. Having these warnings back on will help improve code quality and reduce potential scenarios where unpredicted behavior might cause further issues.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@@ -608,7 +608,7 @@ class BaseWrapper : public BaseHolder<TYPE, BASE, DEFAULTVALUE, IS_NULL>
{
return !!(this->m_value != TYPE(value));
}
#ifdef __GNUC__

// This handles the NULL value that is an int and clang
Copy link
Member

Choose a reason for hiding this comment

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

A nit - the comment talks about clang, but it should be changed to "the compiler"

@jkotas
Copy link
Member

jkotas commented Mar 21, 2020

Could you please resolve the merge conflicts?

@ivdiazsa
Copy link
Member Author

Could you please resolve the merge conflicts?

Done! Now, I'm just waiting for the checks done in Azure Pipelines to merge.

@ivdiazsa
Copy link
Member Author

Issues found in Windows-x86 and Linux-x64 builds. Currently investigating them. Also will run builds for the ARM32 and ARM64 architectures to find any hidden issues, and ensure they work correctly as well.

mangod9 and others added 21 commits March 27, 2020 11:43
- add debug information for `Assert` failures in `MaximumOSVersionTest`
- update src/TestingUtils/Microsoft.AspNetCore.Testing/test/MaximumOSVersionTest.cs
  - co-Authored-By: @Tratcher 
- try with VS2019 queues
- skip `MaximumOSVersion` tests on .NET due to xunit issue
  - co-authored-by: @Tratcher 


Commit migrated from dotnet/extensions@26ff835
* Remove Serilog dependency in extensions

* Add xunit logging for shutdown tests

* Need to remove dependency on AspNetCore.Testing and remove DumpCollector

Commit migrated from dotnet/extensions@540b4e8
…x-logging-event-source

Fix LoggingEventSource to handle null strings


Commit migrated from dotnet/extensions@8d0fa05
…dotnet/extensions#2940)

* Add environment var to allow disabling live reload in default builder

* Switch to using hostingContext, rename flag to fit nomenclature.

* Change flag to memory data source. Change content root default instead of runtime default.

* Update config key to be hierarchical. Change await on positive case to be longer and cancel using the reload token.

* Uncomment a development test case.

* Apply suggestions from code review

Co-authored-by: Chris Ross <Tratcher@Outlook.com>


Commit migrated from dotnet/extensions@64140f9
…y with mono_gchandle_new_weakref_internal. (#34153)

Looks like this was missed in: Unity-Technologies/mono@941a335

Co-authored-by: UnityAlex <UnityAlex@users.noreply.github.com>
Because the formerly uninitialized scratch context is now an input.
Co-authored-by: BrzVlad <BrzVlad@users.noreply.github.com>
Keeping the download of corelib artifacts to check if just corelib is sufficient to build libraires.
…34201)

* Unwrap the TargetInvocationException thrown during an IDispatch.
   This matches the behavior of .NET Framework.
* Add notifyInstructionSetUsage api to jit interface- Add api to be used when the jit is to notify the VM of any usage of higher level instruction sets- Note that this change does nothing using the api. This is merely a scaffolding change to prepare for functional changes later  - There is also scaffoling in the jit to JITDUMP any usage of the api

* Code review feedback
…4221)

This helper is idempotent and exception free, so enable it for value numbering.
Also, no need to spill the entire eval stack for a virtual stub calls.

Addresses part of #7723.
Revert the warn as error as it's causing the coreclr outerloop build to
break. Investigated fixing the warnings but there are a mix of C# and IL
warnings. The latter I'm less sure of the fix. Reverting to unblock and
will investigate the warnings in parallel.

closes #34220
@ivdiazsa
Copy link
Member Author

ivdiazsa commented Apr 7, 2020

Please ignore the commits not made by me. Those are already in master and are showing up here because I had to do a rebase, since the Azure Pipelines kept showing unrelated errors.

@ivdiazsa ivdiazsa closed this Apr 7, 2020
@ViktorHofer ViktorHofer moved this from In progress to Done in Infrastructure Backlog CoreClr Apr 15, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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.

None yet