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

Define SI_NO_CONVERSION by default #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Jun 3, 2024

Since ConvertUTF.h is not installed from CMake, SI_NO_CONVERSION should also be set, otherwise SimpleIni.h is not usable as it tries to include non-existing ConvertUTF.h.

@brofield
Copy link
Owner

brofield commented Dec 1, 2024

I think that making SI_NO_CONVERSION the default on Linux/Mac would be more sensible, although that would be a breaking change compared to earlier versions of the library which I have tried to avoid. I doubt that there are many people using wide char on Linux.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Dec 2, 2024

Not sure which breakage you refer to - the library is already broken and cannot be used without a fix.
The consumers would either need to define SI_NO_CONVERSION, or install ConvertUTF.h bypassing the build system, which doesn't look like a supported configuration.

@brofield
Copy link
Owner

brofield commented Dec 2, 2024

It's not broken, possibly just a bit inconvenient that you need to set the conversion define yourself before header include (as per the docs).

I haven't added the cmake harness, it has been contributed by others, and so if they aren't using the generic conversion, they haven't thought about how ConvertUTF.h/c should be included in the package. It is supported, so how should it be packaged with the cmake harness? Would you like to contribute that?

@brofield brofield closed this Dec 5, 2024
@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Dec 6, 2024

Are you suggesting to include ConvertUTF.[ch] and no longer package as a header-only library, but provide shared/static library instead?

@brofield
Copy link
Owner

brofield commented Dec 9, 2024

ConvertUTF.c/h has always been a part of the library for use if the user wants to use wchar_t as the interface, and needs a library to convert, or wants to use the same library for conversion on all platforms. The reason for inclusion is lost in 15 year old history of this library starting in a commercial product before being open sourced.

I think that these days, no-one should require it. Anyone using just linux will likely be using utf-8 internally, anyone using just Windows will just use the win32 API, and anyone building multi-platform should make their life easier by using a cross platform library like ICU. That said, it has always been there, and just in case there are people using it (which because I get pull requests for changes to it occasionally, I think it might be), I should retain it in the library.

I've already changed the default to be NO_CONVERSION in the header file. The only remaining change is to define what happens at compile time. I think if SI_CONVERT_GENERIC is defined at cmake time, simpleini is built as a static lib with the ConvertUTF.c/h files built and included in the package, if it is not defined then only the SimpleIni.h header file is included as at present. What do you think? If you agree, would you please submit that as a change to the cmake build harness?

@brofield brofield reopened this Dec 9, 2024
@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Dec 9, 2024

I think if SI_CONVERT_GENERIC is defined at cmake time

I am not going to change anything related to bundling simpleini, I'm only interested in systemwide installation. If you have added NO_CONVERSION by default then the library is usable by default (I'll theck this), probably nothing else needs to be changed, apart from a new tagged release is required.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Dec 9, 2024

I'll theck this

Confirmed, master branch works out of box with all consumers I have. So please just tag a release. It's needed to have aeacf86 as well (which I have to apply to 4.22 as a patch).

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