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

Improve/unify approach to line indexing. #233

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Improve/unify approach to line indexing. #233

merged 1 commit into from
Jan 26, 2024

Conversation

mity
Copy link
Owner

@mity mity commented Jan 26, 2024

This PR is an alternative solution to issue addressed in #232.

  • 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.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (aeddaf5) 91.80% compared to head (d52961f) 91.76%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/md4c.c 96.77% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@mity mity changed the title Improve/unify approach to line indexing. [WIP] Improve/unify approach to line indexing. Jan 26, 2024
* 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>
@mity mity changed the title [WIP] Improve/unify approach to line indexing. Improve/unify approach to line indexing. Jan 26, 2024
@ec1oud
Copy link
Contributor

ec1oud commented Jan 26, 2024

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?

@mity
Copy link
Owner Author

mity commented Jan 26, 2024

will also verify that the warnings are gone on Windows.

I've reproduced the warnings locally in MSVC 2019 w/o the patch and the patch fixed those for me.

You are probably planning to have another release soon anyway, right?

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.

@mity mity merged commit 3e8048d into master Jan 26, 2024
11 of 12 checks passed
@ec1oud
Copy link
Contributor

ec1oud commented Jan 26, 2024

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 int n_lines arguments were causing most of the noise, I just changed them all, which is why that patch got to be a bit too much; and indeed it's a good idea to use your MD_SIZE type so that it can be redefined if necessary on some platforms.

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.

@mity mity deleted the fix_line_indexing branch February 4, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants