mity / md4c

C Markdown parser. Fast. SAX-like interface. Compliant to CommonMark specification.
MIT License
755 stars 138 forks source link

Improve/unify approach to line indexing. #233

Closed mity closed 5 months ago

mity commented 5 months ago

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

This resolves some warnings in MSVC builds.

codecov[bot] commented 5 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ec1oud commented 5 months ago

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 commented 5 months ago

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 (https://github.com/google/oss-fuzz/issues/11543). I'd love to resolve it one way or another before the next release if possible.

ec1oud commented 5 months ago

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.