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

c99 fixes #2303

Merged
merged 6 commits into from Aug 6, 2021
Merged

c99 fixes #2303

merged 6 commits into from Aug 6, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

#2302 reports a compilation failure on RHEL6, RHEL7, and SLES12. These are older distros (RHEL7 is running gcc 4.8) which seem to compile in a C90-ish mode by default that gives errors on C99-isms.

This PR removed the C99isms (which are errors) as well as some C90isms (which generate warnings). Specifically:

  • don't declare a variable in a for statement (C99-ism)
  • don't mix declarations and code (C90-ism)

This PR also cleans up some C casting related to how we're handling const qualifiers. Specifically:

  • gumbo.c is now using (const xmlChar *) in place of BAD_CAST (which is an alias for (xmlChar *))
  • extracted the const-discard hack (xmlChar *)(uintptr_t)ptr into a macro DISCARD_CONST_QUAL
  • introduce use of DISCARD_CONST_QUAL in one additional place in xml_node.c

As a result, even when using -Wcast-qual on older Rubies that support it, we now see no compiler warnings.

Have you included adequate test coverage?

I considered setting up CI using a comparable Centos docker image, but I didn't feel like there was much regression value to that test, given the nature of this failure. I could be argued into doing it, if anyone feels strongly.

Does this change affect the behavior of either the C or the Java implementations?

No functional changes.

@flavorjones flavorjones merged commit efe8c0a into main Aug 6, 2021
@flavorjones flavorjones deleted the 2302-c99-fixes branch August 6, 2021 05:22
@flavorjones flavorjones added this to the v1.12.x patch releases milestone Aug 6, 2021
@flavorjones flavorjones added the topic/installation Installation difficulties label Aug 6, 2021
Copy link
Contributor

@stevecheckoway stevecheckoway left a comment

Choose a reason for hiding this comment

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

FWIW, I think this is not a good change. C99 is now more than 20 years old. Unless we're really targeting a compiler that doesn't support it (and we can't be because gumbo itself makes heavy use of C99 features), we should figure out how to pass -std=c99 to the compiler.

(If there were features of C11 that I thought were useful, I'd say we should use -std=c11, but there really aren't for this.)

@stevecheckoway
Copy link
Contributor

That should have referred only to 3e3485b and not the PR as a whole.

@stevecheckoway
Copy link
Contributor

Does adding

append_cflags("-std=c99")

here not work?

@flavorjones
Copy link
Member Author

Hi Steve, thanks for bringing this up, and I'm sorry I didn't wait for your input before whacking the "merge" button. I almost entirely agree with you despite having introduced the change.

But I will point out that @nobu has put effort into keeping the codebase of Ruby, Nokogiri, and other gems "clean" and removing C90-isms because (I think) Ruby 2.6 and earlier still use them, and I want to respect his wishes despite not fully understanding them. I've tagged Nobu here in case he has additional insight.

My only hesitation about passing -std=c99 or -std=c11 is that I have an incomplete understanding of compiler behavior, and I was worried that it might break compilation for some obscure platform. But your point about libgumbo using it is a good one and I'm thinking about whether to revert it. I'll prep that revert/c99 change in a separate PR and wait a bit to see if Nobu has an opinion.

@stevecheckoway
Copy link
Contributor

Looks like -pedantic was removed from Ruby (by @nobu) in 2010 (although I don't claim to understand Ruby's build process).

It looks like -std=gnu99 was added some point before Ruby 2.6. But again, this area is confusing.

flavorjones added a commit that referenced this pull request Aug 6, 2021
@flavorjones flavorjones mentioned this pull request Aug 6, 2021
@flavorjones
Copy link
Member Author

See #2304

flavorjones added a commit that referenced this pull request Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/installation Installation difficulties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants