-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Reenable 4244 #68615
Reenable 4244 #68615
Conversation
Tagging subscribers to this area: @hoyosjs Issue DetailsThis reenables 4244 in all places other than Contributes to #66154 I broke down the updates by general area. @Maoni0 @PeterSolMS See the GC commit @jkotas @davidwrighton @elinor-fung Eyes on the rest are most welcome. @am11 @janvorli I updated zlib-intel. Is there an upstream I can create a PR for?
|
Upstream info of external libs are in their version files:
Note that @lambdageek is in the process of updating zlib and zlib-intel: #68219. |
@am11 and @janvorli I've updated the cmake files for zlib and zlib-intel to suppress 4244. This will likely need to be discussed with @GrabYourPitchforks at some point. |
src/coreclr/tools/superpmi/superpmi-shared/methodcontextreader.cpp
Outdated
Show resolved
Hide resolved
The (new) SuperPMI change LGTM. |
@TIHan PTAL at ilasm changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes under src/coreclr/debug lgtm
@dotnet/runtime-infrastructure Any thoughts on why the above libraries test leg keeps timing out? I'm trying to investigate but I've not found a clear way to get at a root cause. |
@AaronRobinsonMSFT if you look at the 'Tests' page, you can see all the files from the failed tests / work items. For example, Common.Tests has runclient.py with:
|
@elinor-fung Thanks. The following doesn't seem like it is anything I've done:
|
The timeout/failure above is tracked in #67728. |
@jkotas @Maoni0 @PeterSolMS @elinor-fung @TIHan Can you please take another look at this? This would finish off the current SDL requirements for the repo outside of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
This seems to be a failure with uploading logs. The tests appear to have all passed. /cc @dotnet/runtime-infrastructure
|
This reenables 4244 in all places other than
mono/
Contributes to #66154
I broke down the updates by general area.
@Maoni0 @PeterSolMS See the GC commit
@janvorli There is a single exception walk logic update. See the Debugger commit
@BruceForstall There is a single SuperPMI update - See the SuperPMI commit
@jkotas @davidwrighton @elinor-fung Eyes on the rest are most welcome.
@am11 @janvorli I updated zlib-intel. Is there an upstream I can create a PR for?