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

fix a bunch of -Wall warnings #592

Merged
merged 12 commits into from
Jul 29, 2024

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Jul 28, 2024

Rehash of #579, only the uncontroversial stuff, with amended commit messages.


This change is Reviewable

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

Pinging @NiLuJe for qimagescale.cpp quick review.

crengine/src/epubfmt.cpp Outdated Show resolved Hide resolved
crengine/src/hist.cpp Show resolved Hide resolved
@poire-z poire-z merged commit 159dba1 into koreader:master Jul 29, 2024
1 check passed
@poire-z
Copy link
Contributor

poire-z commented Jul 29, 2024

CI suceeded on master after this is merged, even though part 2 was not yet merged.
I'm ok with that success :) but when looking at the output:
https://github.com/koreader/crengine/actions/runs/10140113071/job/28034656717
items with warnings do not standout vs. those without (no opening, no color). Is that expected ?
Meaning I'll open each of the item to look only twice a year - which is fine by me :)

@Frenzie
Copy link
Member

Frenzie commented Jul 29, 2024

It's probably intentional until that is merged so the warnings can be turned on without being distracting noise.

@benoit-pierre benoit-pierre deleted the pr/fix_-Wall_warnings_part1 branch July 29, 2024 20:43
@benoit-pierre
Copy link
Contributor Author

The thinking was more along the lines of: suppress spurious warnings, and rely on annotations. I need to check how multi-line matchers work, see if we can get all the details of a warning (e.g. the following reasoning notes by clang-tidy) in the annotation.

@benoit-pierre
Copy link
Contributor Author

Note: there's also the summary in the workflow.

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.

3 participants