Skip to content

Fixed pedantic toolchain warnings.#229

Open
malyjak wants to merge 8 commits intoyaml:masterfrom
malyjak:feat/jma/PedanticFixes
Open

Fixed pedantic toolchain warnings.#229
malyjak wants to merge 8 commits intoyaml:masterfrom
malyjak:feat/jma/PedanticFixes

Conversation

@malyjak
Copy link
Copy Markdown

@malyjak malyjak commented Oct 5, 2021

Our toolchain is very pedantic. I fixed several issues it pointed out.

Copy link
Copy Markdown

@pantoniou pantoniou left a comment

Choose a reason for hiding this comment

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

Minor changes requested to get rid of casts. Otherwise looks good.

Comment thread tests/run-dumper.c Outdated
Comment thread tests/run-emitter.c Outdated
Comment thread tests/test-reader.c Outdated
}
else {
if (parser.unread != check) {
if ((int)parser.unread != check) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here, avoid casts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The cast still needs to be there, to avoid compiler warnings. However, I improved the logic. Please see the new line 187.

Comment thread src/api.c Outdated

if ((char *)*end - (char *)*start >= INT_MAX / 2)
return 0;
return 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn't the indent broken?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, it is. Another reason to fully use 4 spaces / tab / {}, so autoformating tools will not struggle

@JKorbelRA
Copy link
Copy Markdown

JKorbelRA commented Oct 19, 2022

Can this go into the master yet? Is there still something remaining, @nobu @pantoniou ? I can help/fix that, if needed.

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.

4 participants