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.

Sign up for GitHub

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

Jump to bottom

Fix or disable warnings on latest VS dogfood #49799

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Fix or disable warnings on latest VS dogfood #49799

merged 2 commits into from
Mar 19, 2021

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Mar 18, 2021

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 18, 2021
@jkotas jkotas requested a review from BruceForstall March 18, 2021 06:59
@jkotas
Copy link
Member Author

jkotas commented Mar 18, 2021

cc @dotnet/jit-contrib

jkotas
jkotas commented Mar 18, 2021
View reviewed changes
src/coreclr/jit/codegencommon.cpp
@@ -11409,7 +11408,6 @@ void CodeGen::genRegCopy(GenTree* treeNode)
//
// There should never be any circular dependencies, and we will check that here.

GenTreeCopyOrReload* copyNode = treeNode->AsCopyOrReload();
Copy link
Member Author

Choose a reason for hiding this comment

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

If some of these locals exist to aid debugging, I am happy to revert. Just let me know...

@jkotas jkotas mentioned this pull request Mar 18, 2021
Closed
sandreenko
sandreenko approved these changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@sandreenko sandreenko 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

src/coreclr/jit/lsra.cpp Show resolved Hide resolved
eng/native/configurecompiler.cmake Outdated
@@ -502,6 +502,13 @@ if (MSVC)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/Zc:inline>) # All inline functions must have their definition available in the current translation unit.
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/Zc:forScope>) # Enforce standards-compliant for scope.

add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/wd4065>)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference between this block and the block below # Disable Warnings:?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know.

BruceForstall
BruceForstall approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Thanks!

IMO, it would be useful to comment for each /wd what the text of the warning is that is being disabled. Otherwise, it's just a big list and nobody goes to the docs to see why there are so many warnings disabled.

@jkotas
Copy link
Member Author

jkotas commented Mar 18, 2021

comment for each /wd what the text of the warning is that is being disabled

Done. Also, deleted a few /wd that do not seem to be necessary anymore.

@jkotas jkotas closed this Mar 18, 2021
@jkotas jkotas reopened this Mar 18, 2021
@jkotas jkotas closed this Mar 18, 2021
@jkotas jkotas reopened this Mar 18, 2021
@BruceForstall
Copy link
Member

@jkotas fyi, the JIT has it's own set of disabling pragmas,

runtime/src/coreclr/jit/jit.h

Lines 28 to 45 in 705daa1

#ifdef _MSC_VER
// These don't seem useful, so turning them off is no big deal
#pragma warning(disable : 4065) // "switch statement contains 'default' but no 'case' labels" (happens due to #ifdefs)
#pragma warning(disable : 4510) // can't generate default constructor
#pragma warning(disable : 4511) // can't generate copy constructor
#pragma warning(disable : 4512) // can't generate assignment constructor
#pragma warning(disable : 4610) // user defined constructor required
#pragma warning(disable : 4211) // nonstandard extension used (char name[0] in structs)
#pragma warning(disable : 4127) // conditional expression constant
#pragma warning(disable : 4201) // "nonstandard extension used : nameless struct/union"
// Depending on the code base, you may want to not disable these
#pragma warning(disable : 4245) // assigning signed / unsigned
#pragma warning(disable : 4146) // unary minus applied to unsigned
#pragma warning(disable : 4100) // unreferenced formal parameter
#pragma warning(disable : 4291) // new operator without delete (only in emitX86.cpp)
#endif
. Probably don't need to touch these.

@jkotas
Copy link
Member Author

jkotas commented Mar 18, 2021

JIT has it's own set of disabling pragmas,

The global warning disables kick for the JIT as well. I think the items in this list are either redundant with the global list or not needed anymore. I have tried to delete. Let's see what CI thinks.

@jkotas
Copy link
Member Author

jkotas commented Mar 19, 2021

This change is affecting Windows only. All Windows CI legs passed.

@jkotas jkotas merged commit c999ed4 into dotnet:main Mar 19, 2021
@jkotas jkotas deleted the fix-warnings branch March 19, 2021 03:20
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sandreenko sandreenko sandreenko approved these changes

@BruceForstall BruceForstall BruceForstall approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jkotas @BruceForstall @stephentoub @sandreenko @karelz