mayah / tinytoml

A header only C++11 library for parsing TOML
BSD 2-Clause "Simplified" License
167 stars 31 forks source link

Optional indentation + move semantics #12

Closed sir-earl closed 8 years ago

sir-earl commented 8 years ago

Add the ability to format output with indentation if required.

Example usage: value.write(&os, toml::Value::INDENT_ENABLED);

Also added move semantics for a small performance increase.

mayah commented 8 years ago

Thanks for the pull request. Unfortunately this doesn't compile. move() should be std::move().

mayah commented 8 years ago

I've added a few comments. If you could address, I'd like to merge your pull request. Otherwise, maybe I'll change the code by my side.

Again, thanks for the pull request!

sir-earl commented 8 years ago

I've made the changes you suggested, but I've also added a new FormattedValue wrapper class for passing in formatting options:

os << toml::FormattedValue(value, toml::FORMAT_INDENT);

I'm happy to make any further changes you suggest, or if you'd like to go in a different direction with the formatting options, let me know.

Cheers!

mayah commented 8 years ago

Thanks for the pull request! I'm OK with version 12f6849.

I need to think about FormattedValue more to merge it. I feel holding reference could be error prone; it easily causes memory violation. Let me give time to consider a bit. It would be good to separate the last commit from this pull request.

sir-earl commented 8 years ago

Fair point. I was looking into stream manipulators, which might be a good way to do this, but they're quite complicated looking, and I don't have the time to mess about with them.

How about I add a 'Value::writeFormatted' method to replace the FormattedValue class? The downside is that it obviously can't be used with stream operator <<.

mayah commented 8 years ago

I'm going to merge this manually. I merge several commits to one by feature

mayah commented 8 years ago

I manually merged this with 3 commits. Thanks for the pull request. Closing.