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

Migrate from SimpleIni to our own implementation #7511

Merged
merged 4 commits into from
Nov 5, 2024
Merged

Conversation

glebm
Copy link
Collaborator

@glebm glebm commented Nov 5, 2024

Our implementation has a more modern interface and only supports the features that we care about.

It always outputs \r\n as newlines and does not output BOM.

The modern interface eliminates awkward c_str()/data() conversions.

This implementation preserves comments and the file order of sections and keys. New keys are written in insertion order.

We now also support modifying and adding default comments, which may be a useful thing to do for the especially tricky ini options (this PR doesn't add any but adds the ability to do so).

Sadly, this increases the RG99 binary size by 24 KiB. I'm guessing this is because the map implementation generates quite a bit of code.

Note that while it might seem that using std::string for every key and value would do a lot of allocations, most of these strings are small and thus benefit from Small String Optimization (= no allocations).

@glebm glebm force-pushed the ini branch 3 times, most recently from a0c116b to 3bd9980 Compare November 5, 2024 01:24
@glebm glebm enabled auto-merge (rebase) November 5, 2024 01:27
glebm added 3 commits November 5, 2024 01:59
Our implementation has a more modern interface and only
supports the features that we care about.

It always outputs `\n` as newlines and does not output BOM.

The modern interface eliminates awkward `c_str()/data()` conversions.

This implementation preserves comments and the file order of sections
and keys. New keys are written in insertion order.

We now also support modifying and adding default comments,
which may be a useful thing to do for the especially tricky
ini options (this PR doesn't add any but adds the ability to do so).

Sadly, this increases the RG99 binary size by 24 KiB.
I'm guessing this is because the map implementation generates
quite a bit of code.

Note that while it might seem that using `std::string` for every key and
value would do a lot of allocations, most of these strings are
small and thus benefit from Small String Optimization (= no allocations).
Some platforms do not have the C++20
`string_view(begin, end)` constructor.
Appears to be unimplemented on several platforms
@AJenbo
Copy link
Member

AJenbo commented Nov 5, 2024

It always outputs \n as newlines

Considering INI files are a Windows thing and we extend diablo.ini maybe it should be \r\n?

@AJenbo
Copy link
Member

AJenbo commented Nov 5, 2024

I never was particularly happy with any of the ini libs. I like the cleaner options code, was this the main reason for the change?

@glebm
Copy link
Collaborator Author

glebm commented Nov 5, 2024

Considering INI files are a Windows thing and we extend diablo.ini maybe it should be \r\n?

SimpleIni produced \r\n on Windows and \n on other platforms.
I wanted to make it consistent on all platforms (whichever way we decide on).

Note that INI libraries on Windows support \n, and even Notepad displays \n correctly these days.

I like the cleaner options code, was this the main reason for the change?

Yeah, I wanted to clean that up a bit and figured writing an INI library from scratch won't be that difficult.

@AJenbo
Copy link
Member

AJenbo commented Nov 5, 2024

I like consistency, I think we should go for the Windows endings since it's often accessed my non technical users and it's what it's been for most users and we don't know how diablo reacts if it's files was changed to the other format.

@glebm
Copy link
Collaborator Author

glebm commented Nov 5, 2024

Done, switched to \r\n

@glebm glebm merged commit 1892f6a into diasurgical:master Nov 5, 2024
23 checks passed
@glebm glebm deleted the ini branch November 5, 2024 10:12
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