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

resolves #2441 #2464

Closed
wants to merge 1 commit into from
Closed

Conversation

stuartcarnie
Copy link
Contributor

Convert ext-iconv to HNI

@sgolemon sgolemon added this to the under review milestone Apr 16, 2014
@sgolemon sgolemon self-assigned this Apr 16, 2014
@sgolemon
Copy link
Contributor

Made some minor edits on the way in.

  • Killed ext_iconv.h - Nobody needs this file, and with static linkage they wouldn't be able to use it anyway
  • Made string constants use global scope static strings - Don't want these destructing (not that they will currently, but things may change)
  • Various lint fixed (trailing whitespace, 80col limit, etc...)

@JoelMarcey
Copy link
Contributor

Internal Diff Number: D1279705

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@JoelMarcey JoelMarcey assigned JoelMarcey and unassigned sgolemon May 20, 2014
@JoelMarcey
Copy link
Contributor

@stuartcarnie Hi. We are close to landing this (we are probably going to land as-is, actually). But, we ran into some weird compilation issues in OSS around INT32_MAX not being in scope. We are still not sure why we have these issues, but it did raise a question.

Why do you have this line in iconv_substr of ext_iconv.cpp

length = length <= INT32_MAX ? length : INT32_MAX;

length is being declared in ext_iconv.php as PHP_INT_MAX which is 64-bit.

Why the downcast to INT32_MAX?

cc:/ @ptarjan

@stuartcarnie
Copy link
Contributor Author

The downcast is because the implementation of _php_iconv_substr requires an int32_t, however upon reviewing the code (on my phone), there is no reason not to change the _php_iconv_* functions to use int64_t

Would you like me to do that - I know Sara has made some tweaks already, so it may not be practical.

@stuartcarnie
Copy link
Contributor Author

It has been a while since I touched this, but I seem to recall I looked at leaving it as int64_t originally but ran into an issue that it wasn't working when JITting the code

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