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

Common options #3204

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented May 24, 2024

What problem is this PR intended to solve?

See discussion at #3199

cc @stevecheckoway

@flavorjones
Copy link
Member Author

There's a downstream failure with the Sanitize gem: https://github.com/sparklemotion/nokogiri/actions/runs/9226924515/job/25387760706?pr=3204

I'll take a look.

@flavorjones
Copy link
Member Author

Ah, I think it's because of signedness, see compiler warnings:

../../../../ext/nokogiri/gumbo.c:572:51: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
  572 |   options.max_tree_depth = options.max_tree_depth < 0 ? -1 : (options.max_tree_depth + 1);
      |                                                   ^
../../../../ext/nokogiri/gumbo.c:572:57: warning: operand of ‘?:’ changes signedness from ‘int’ to ‘unsigned int’ due to unsignedness of other operand [-Wsign-compare]
  572 |   options.max_tree_depth = options.max_tree_depth < 0 ? -1 : (options.max_tree_depth + 1);
      |                                                         ^~

I'll fix it.

@flavorjones
Copy link
Member Author

Following the yak shave for a moment, this snuck through because it's not possible to compile Ruby extensions with -Wsign-conversion at the moment. I'm going to try to get a patch into Ruby to allow this.

@flavorjones
Copy link
Member Author

@stevecheckoway I don't have write permissions on your branch, so I'm going to close this and open a new one using a copy of the branch that I've pushed to origin.

flavorjones added a commit that referenced this pull request May 24, 2024
**What problem is this PR intended to solve?**

See discussion at #3199 

cc @stevecheckoway 

(Recreation of #3204 with additional commits)
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