Replace boost ini parser

[wheybags]

Boost ini parser is used on startup for settings + when loading CEL files. It would be nice to replace this, as it uses exceptions internally, which completely breaks the ability to set a brakpoint on exceptions being thrown. Using a single-header lib like https://github.com/jtilly/inih would probably be the best option.

[balintkissdev]

Hi,

I looked into this and found out that https://github.com/jtilly/inih has only an INI reader and has no capabilities for modifying and saving an INI file. What I found instead is https://github.com/brofield/simpleini, which is also a header-only library.

I added simpleini into extern/ and started experimenting with it. Exceptions can be avoided by using this. The drawback is that it doesn't have a template-like API like property_tree has, but can be simulated by using template specializations inside settings.cpp. The current work is in https://github.com/balintkiss501/freeablo/tree/388_replace_boost_ini_parser. I had to use PIMPL idiom and hide the usage of simpleini in settings.cpp, otherwise other components using Settings would have to include the simpleini header too, introducing a dependency to simpleini, making the binary more bloated and requiring the modification of CMakeLists.txt.

I'm not finished it yet because

• I hadn't tested it yet
• Not sure if reading hex values work out of the box, have to try it
• I'm planning to write a unit test with GTest to make sure I didn't break the functionality

In the meantime, I would like to ask some questions regarding the design:

1. How would you like simpleini to be included in extern/?

• Commit the simpleini folder as it is
• Add simpleini as a git submodule, but then README has to be modified
• Or add it as git submodule and make CMake download it?

2. Which style would you prefer? Returning the result of a function directly like this

template <> std::string Settings::get<std::string>(const std::string& section, const std::string& name, std::string defaultValue)
{
    return mImpl->mUserPropertyTree.GetValue(section.c_str(), name.c_str(), defaultValue.c_str());
}

or storing the result int a variable to make debugging easier?

template <> std::string Settings::get<std::string>(const std::string& section, const std::string& name, std::string defaultValue)
{
    const std::string value = mImpl->mUserPropertyTree.GetValue(section.c_str(), name.c_str(), defaultValue.c_str());
    return value;
}

3. I disabled the copying of Settings because I didn't see it being copied anywhere and I didn't feel comfortable copying around open INI file handles. Would you like me to implement deep copy for the PIMPL variable anyway?

[wheybags]

How would you like simpleini to be included in extern/? My preference would be to add it as a git subtree (https://www.atlassian.com/blog/git/alternatives-to-git-submodule-git-subtree), then add cmake code for building it.

As for the other questions, I don't mind really, whatever works. Thanks for the work so far!

On Sun, Nov 18, 2018, at 1:31 PM, Bálint Kiss wrote:

Hi,

I looked into this and found out that https://github.com/jtilly/inih has only an INI reader and has no capabilities for modifying and saving an INI file. What I found instead is https://github.com/brofield/simpleini, which is also a header-only library.

I added simpleini into extern/ and started experimenting with it. Exceptions can be avoided by using this. The drawback is that it doesn't have a template-like API like property_tree has, but can be simulated by using template specializations inside settings.cpp. The current work is in https://github.com/balintkiss501/freeablo/tree/388_replace_boost_ini_parser. I had to use PIMPL idiom and hide the usage of simpleini in settings.cpp, otherwise other components using Settings would have to include the simpleini header too, introducing a dependency to simpleini, making the binary more bloated and requiring the modification of CMakeLists.txt.

I'm not finished it yet because

• I hadn't tested it yet
• Not sure if reading hex values work out of the box, have to try it
• I'm planning to write a unit test with GTest to make sure I didn't break the functionality

In the meantime, I would like to ask some questions regarding the design:

1. How would you like simpleini to be included in extern/?

• Commit the simpleini folder as it is
• Add simpleini as a git submodule, but then README have to be modified
• Or add it as git submodule and make CMake download it?

2. Which style would you prefer? Returning the result of a function directly like this

template <> std::string Settings::get<std::string>(const std::string& 
section, const std::string& name, std::string defaultValue)
{
    return mImpl->mUserPropertyTree.GetValue(section.c_str(), 
name.c_str(), defaultValue.c_str());
}

or storing the result int a variable to make debugging easier?

template <> std::string Settings::get<std::string>(const std::string& 
section, const std::string& name, std::string defaultValue)
{
    const std::string value = mImpl-
>mUserPropertyTree.GetValue(section.c_str(), name.c_str(), 
defaultValue.c_str());
    return value;
}

3. I disabled the copying of Settings because I didn't see it being copied anywhere and I didn't feel comfortable copying around open INI file handles. Would you like me to implement deep copy for the PIMPL variable anyway?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/wheybags/freeablo/issues/388#issuecomment-439689393

[tilkinsc]

Are you guys phasing out boost?

[wheybags]

That might be nice, but no, not for now.

On Wed, Jan 2, 2019, at 8:39 AM, Cody Tilkins wrote:

Are you guys phasing out boost?

-- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: https://github.com/wheybags/freeablo/issues/388#issuecomment-450801432