-
Notifications
You must be signed in to change notification settings - Fork 147
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
Improve/unify approach to line indexing. #233
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #233 +/- ##
==========================================
- Coverage 91.80% 91.76% -0.05%
==========================================
Files 5 5
Lines 3406 3411 +5
==========================================
+ Hits 3127 3130 +3
- Misses 279 281 +2 ☔ View full report in Codecov by Sentry. |
fcd3be0
to
4229859
Compare
* Use consistently type MD_SIZE for line indeces. * Remove pointer arithmetic if lines and replace it with line index arithmetic. This resolves some warnings in MSVC builds. See PR #232. Co-authored-by: Martin Mitas <mity@morous.org> Co-authored-by: Shawn Rutledge <s@ecloud.org>
4229859
to
d52961f
Compare
OK. I think Qt wants to follow md4c releases for the bundled version of md4c (not sure); but this patch doesn't apply cleanly on top of 0.5.1. But I can run a version with all recent changes through CI (to run our autotests), and will also verify that the warnings are gone on Windows. You are probably planning to have another release soon anyway, right? |
I've reproduced the warnings locally in MSVC 2019 w/o the patch and the patch fixed those for me.
I have no exact schedule. After merging this PR I'd like to let things settle down little bit and see there are no new issues from fuzzing. There's also still one odd report from oss-fuzz with sort of contradictory metadata so it's not clear whether it's genuine bug in MD4C or rather an artifact/issue in their database or something like that (google/oss-fuzz#11543). I'd love to resolve it one way or another before the next release if possible. |
Yeah. It also concerns me to have Qt shipping such new code with so many changes, even in patch releases. But management thinks that if (hypothetically) vulerabilities are found, it's easier to apply small fixes on top of recent code rather than trying to catch up with bigger changes just to fix that; and so on. So we have a blanket policy that we keep our dependencies updated; and so they wanted me to update the bundled version in Qt 6.6.2 to 0.5.1, just because it's the newest release. (In fact Arch Linux already updated the package, so that means even their Qt 6.6.1 is already using your newest release, as a shared library rather than statically linked. If every OS that Qt supports was doing it that way, we wouldn't bundle md4c: just let it be an external dependency.) Then a colleague noticed the msvc warnings. And after I saw that I think we both should be careful about releases: my instinct is that we should confine new versions of md4c to new minor versions of Qt (as long as there's no need to fix specific bugs), rather than point releases on old branches. But that's not what we're doing, and neither is Arch. It's good that you do fuzzing (so do we), and that takes time. So I don't know... either we will tolerate the msvc warnings for a while, or update md4c again if there is a release soon, or we need a warning-fix patch on 0.5.1. |
This PR is an alternative solution to issue addressed in #232.
MD_SIZE
for line indeces.This resolves some warnings in MSVC builds.