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

Add iconv functions #109

Conversation

suve
Copy link
Collaborator

@suve suve commented Jan 13, 2023

This changeset adds SDL_iconv* functions and related types.

Note: Since many other symbols found in SDL_stdinc.h are missing from our sdlstdinc.inc, some changes have been made:

  1. FPC's built-in Strings.strlen() is called instead of SDL_strlen(), where approriate.
  2. The SDL_iconv_wchar_utf8() macro is not included in this changeset, as it requires SDL_wcslen().

@Free-Pascal-meets-SDL-Website
Copy link
Collaborator

Thanks!

Sorry for the delayed response.

The following points I like to hint to or ask about:

  1. I'd prefer lower-case const keyword (general code style convention).
  2. The identation for the Result statements in the three SDL_iconv_* macros is not equal.
  3. I'd like to ask why you go for the Strings unit implementation of strlen instead of adding/using SDL_StrLen? For me the latter solution seems more appropriate here.

Best regards
Matthias

@suve
Copy link
Collaborator Author

suve commented Jan 31, 2023

I'd like to ask why you go for the Strings unit implementation of strlen instead of adding/using SDL_StrLen? For me the latter solution seems more appropriate here.

We already do something similar for SDL_FRectEqualsEpsilon(), where we call FPC's System.Abs() instead of SDL_fabsf(). (link)

I've asked this in #62 but didn't really receive an answer - do we want to go all-in and add definitions for all the cstdlib-like SDL functions? If so, then I'll follow up this PR with another one (or more) adding stdinc.h symbols and fixing any relevant macros.

@Free-Pascal-meets-SDL-Website
Copy link
Collaborator

We already do something similar for SDL_FRectEqualsEpsilon(), where we call FPC's System.Abs() instead of SDL_fabsf(). (link)

I've asked this in #62 but didn't really receive an answer - do we want to go all-in and add definitions for all the cstdlib-like SDL functions? If so, then I'll follow up this PR with another one (or more) adding stdinc.h symbols and fixing any relevant macros.

Thanks for the elaborated answer.

I see your point, the main reason why I stumbled about this, is the introduction of the Strings unit, which could be prevented when using SDL's own function. This would be desirable to have less dependency on certain units and higher compatibility between compilers. It seems Delphi has no Strings unit but a AnsiStrings unit for StrLen. Also, FPC and Delphi have strlen as part of their SysUtils unit (see FPC SysUtils.strlen and Delphi SysUtils.StrLen), which would be more compatible as we don't have to distinguish between the used units based on the compiler. - Unfortunately Delphi marked the SysUtils.StrLen as deprecated.

Please fix this PR to be Delphi-compatible.

I'm sorry, I forgot about your reply in #62 . Let's discuss this in general there. But to answer your question

do we want to go all-in and add definitions for all the cstdlib-like SDL functions?
here, which is highly related to the fix-up,

I would suggest to go for addition of cherry-picked, useful SDL functions if it is worth it. To my mind it is worth it, if we can prevent adding new units (dependencies).

Best regards
Matthias

@suve
Copy link
Collaborator Author

suve commented Mar 9, 2023

Thanks for the insight regarding Delphi compatibility. I've pushed one more commit that should include the proper unit based on the compiler used. Sorry for the long wait.

I'll follow this up with a couple of PRs introducing some cherry-picked functions from SDL_stdint.h.

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.

None yet

2 participants