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

Refactor compiler diagnostics and deprecations #1270

Closed
wants to merge 1 commit into from

Conversation

KorigamiK
Copy link

This is my attempt to get sioyek to compile with without errors and warnings from clang.

The changes help fix deprecation warnings while compiling against Qt 6.8, compiler warnings and potential errors.

We go from around 50+ diagnostics down to 1 so thats good. I currently didn't apply any logical changes so we should be fine.
Please let me know about the review

@ahrm
Copy link
Owner

ahrm commented Dec 29, 2024

Thanks, some of these fix genuine bugs (e.g. missing \ in some regular expressions), but I don't care about most of the style warnings. For that reason I will not merge this pr but I will fix the parts where there are actual problems.

@ahrm ahrm closed this Dec 29, 2024
@KorigamiK
Copy link
Author

Although I have made sure to not include any stylistic changes and only apply changes to potential bugs and deprecation updates, you are feel to apply these changes as needed.
Also I think I made some changes to input.cpp that were unnecessary (adding overrides), so I've fixed them in my last commit which you can check out here KorigamiK@7a5b030

I can create a new pr if needed

ahrm added a commit that referenced this pull request Dec 29, 2024
@ahrm
Copy link
Owner

ahrm commented Dec 29, 2024

Thanks, I included most of the suggested changes in ed14e38.

This suggestion is wrong though:

            if ((chars_between_last_dot_and_index[i] > 0) && (chars_between_last_dot_and_index[i] < 128) && std::isalpha(chars_between_last_dot_and_index[i])) {
            if ((chars_between_last_dot_and_index[i] > 0) && std::isalpha(chars_between_last_dot_and_index[i])) {

Changing the upper line to the lower line might cause a crash when dealing with non-ascii characters.

@KorigamiK
Copy link
Author

KorigamiK commented Dec 29, 2024

chars_between_last_dot_and_index[i] < 128

I think a signed char is always less than 128 so this is always true
(I Might be mistaken here though)

@ahrm
Copy link
Owner

ahrm commented Dec 29, 2024

I think a signed char is always less than 128 so this is always true

Not when working with unicode characters.

@KorigamiK
Copy link
Author

KorigamiK commented Dec 29, 2024

I think a signed char is always less than 128 so this is always true

Not when working with unicode characters.

Makes sense, I think the issue might lie in this container definition,

std::vector<char> chars_between_last_dot_and_index;

A larger element type may be needed

@ahrm
Copy link
Owner

ahrm commented Dec 29, 2024

Yes, ideally we would use std::vector<int> in that location, but since in that function we only care about ascii characters it doesn't matter.

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.

2 participants