Skip to content

Remove implicit narrowing conversions from zlib - #91245

Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged

Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom
GrabYourPitchforks:zlib-binskim

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Aug 28, 2023

Copy link
Copy Markdown
Member

Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.

See the individual .patch files for a description of the changes and justification of their correctness.

@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost

ghost commented Aug 28, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.

See the individual .patch files for a description of the changes and justification of their correctness.

Author:GrabYourPitchforks
Assignees:-
Labels:

area-System.IO.Compression

Milestone:-

@GrabYourPitchforks

GrabYourPitchforks commented Aug 28, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.

I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.

This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.

@GrabYourPitchforksGrabYourPitchforks mentioned this pull request Aug 29, 2023
Closed
@build-analysisbuild-analysisBot mentioned this pull request Aug 29, 2023
Closed
@carlossanlop

carlossanlop commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

Is it your intention to get this backported to 8.0?

carlossanlop
carlossanlop approved these changes Aug 29, 2023
View reviewed changes

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.

Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001

@carlossanlopcarlossanlopAug 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?

@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.

Comment threadsrc/native/external/zlib/trees.c
continue;
} else if (count < min_count) {
s->bl_tree[curlen].Freq += count;
s->bl_tree[curlen].Freq += (ush)count;

@carlossanlopcarlossanlopAug 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.

@akoeplingerakoeplingerAug 30, 2023•
edited
Loading

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.

It's from a typedef here:

runtime/src/native/external/zlib/zutil.h

Line 41 in f750bb0

typedefunsigned shortush;

@carlossanlopcarlossanlopAug 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.

@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.

Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2023

Copy link
Copy Markdown
MemberAuthor

Is it your intention to get this backported to 8.0?

Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.

@GrabYourPitchforks
GrabYourPitchforks merged commit d28bac7 into dotnet:mainAug 30, 2023
@GrabYourPitchforks

GrabYourPitchforks commented Aug 30, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@GrabYourPitchforks

GrabYourPitchforks commented Sep 12, 2023

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0

@GrabYourPitchforks
GrabYourPitchforks deleted the zlib-binskim branch September 12, 2023 21:39
@github-actions

github-actionsBot commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907

@github-actionsgithub-actionsBot mentioned this pull request Sep 12, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
+1 more reviewer
@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Remove implicit narrowing conversions from zlib by GrabYourPitchforks · Pull Request #91245 · dotnet/runtime · GitHub
Skip to content
Remove implicit narrowing conversions from zlib - #91245
Merged
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Aug 30, 2023
Merged
Remove implicit narrowing conversions from zlib#91245
GrabYourPitchforks merged 2 commits into
dotnet:mainfrom 
GrabYourPitchforks:zlib-binskim
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Aug 28, 2023

Copy link


Copy Markdown

Member
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
@GrabYourPitchforks
Make zlib compile clean against MSVC C4244
1603fba
and clang implicit-int-conversion
Descriptions of each changes are in the respective .patch files.
@ghost
ghost
commented
Aug 28, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io-compression

See info in area-owners.md if you want to be subscribed.
Issue Details
Allows us to reenable mandatory MSVC C4244 compiler warning and clang's "implicit-int-conversion" warning.
See the individual .patch files for a description of the changes and justification of their correctness.
Author:GrabYourPitchforks
Assignees:-
Labels:
area-System.IO.Compression
Milestone:-
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Testing: On my Windows x64 box, I validated the zlib-intel build under MSVC. Then I changed my local .cmake files to force usage of zlib instead of zlib-intel and validated that it built correctly under MSVC. On my WSL box, I modified my local .cmake files to force it to use these zlib sources instead of the OS's inbox zlib. I also injected an #error line into deflate.c just to confirm that clang was in fact using our local files.
I also do plan on submitting this upstream to both zlib and zlib-intel if it passes review here, since others could also benefit from these changes.
This PR shouldn't introduce any behavioral changes, since it should produce the same AST under the covers. It just makes the casts explicit instead of implicit so that we can re-enable required warnings.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Aug 29, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Aug 29, 2023

Closed

@carlossanlop
carlossanlop
commented
Aug 29, 2023

Copy link


Copy Markdown

Contributor
Is it your intention to get this backported to 8.0?
carlossanlop
carlossanlop
approved these changes
Aug 29, 2023
View reviewed changes
@carlossanlopcarlossanlop
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

No blocking comments, just two questions for my own education and a small suggestion. Thanks for submitting this change.
Comment threadsrc/native/external/patches/zlib-intel/0001-Make-zlib-intel-compile-clean-against-C4244.patch
@@ -0,0 +1,75 @@
From edabaf799fd071a328e0adb743a98628df6649f0 Mon Sep 17 00:00:00 2001
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why is a patch file needed in a PR?
@akoeplingerakoeplingerAug 30, 2023

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.

It makes it easier to separate our customizations so when we import a new upstream version we can just re-apply the patch.
Comment threadsrc/native/external/zlib/trees.c
 continue;
 } else if (count < min_count) {
 s->bl_tree[curlen].Freq += count;
 s->bl_tree[curlen].Freq += (ush)count;
@carlossanlopcarlossanlopAug 29, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

What's ush? Having trouble finding its definition.
@akoeplingerakoeplingerAug 30, 2023•

edited

Loading

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.

It's from a typedef here:
runtime/src/native/external/zlib/zutil.h

Line 41
in
f750bb0
typedefunsigned shortush; 
@carlossanlopcarlossanlopAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks. Oof, I really dislike cryptic c typedefs.
@GrabYourPitchforksGrabYourPitchforksAug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Agreed. It's trying to match the maintainer's style as closely as possible, which is why it could look strange from our eyes.
Comment threadsrc/native/external/zlib-intel/deflate.c
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023

Copy link


Copy Markdown

MemberAuthor
Is it your intention to get this backported to 8.0?
Yes, since TSA will eventually complain if we don't. But honestly it can wait until after RC / RTM if needed.
@GrabYourPitchforks
GrabYourPitchforks
merged commit d28bac7
into
dotnet:mainAug 30, 2023
@GrabYourPitchforks
GrabYourPitchforks
commented
Aug 30, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@GrabYourPitchforks
GrabYourPitchforks
commented
Sep 12, 2023

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0
@GrabYourPitchforks
GrabYourPitchforks
deleted the

zlib-binskim

branch
September 12, 2023 21:39
@github-actions
github-actionsBot
commented
Sep 12, 2023

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165145907
@github-actionsgithub-actionsBot
mentioned this pull request
Sep 12, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Oct 13, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar

+1 more reviewer

@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@GrabYourPitchforksGrabYourPitchforks

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@carlossanlop@akoeplinger