SFML community forums

General => SFML wiki => Topic started by: Foaly on October 16, 2012, 07:46:15 am

Title: Settings parser
Post by: Foaly on October 16, 2012, 07:46:15 am
Hello!
I really like the simplicity of the settings parser on the wiki. (https://github.com/SFML/SFML/wiki/Source%3A-Settings-Parser) I would like to use it in a project and I couldn't find a license notice so I was wondering under what license the code is.
Title: Re: Settings parser
Post by: Laurent on October 16, 2012, 07:58:58 am
People always forget to include a license when they post code on the wiki. So try to contact the author if he left an e-mail address, otherwise just use it freely... that's most likely the intended usage when there is no explicit license (for this wiki, I mean).
Title: Re: Settings parser
Post by: Foaly on October 16, 2012, 09:28:07 am
Alright. I'll try.
Would it maybe be a good idea to make all the codes on the wiki under public domain by default? Meaning if nothing else is specified you can use them for whatever you want? This could prevent license issue in the future or clarify things if the author is unavailable.
Title: Re: Settings parser
Post by: Laurent on October 16, 2012, 09:31:38 am
I guess I could do that, yes.
Title: Re: Settings parser
Post by: Foaly on October 16, 2012, 09:41:03 am
Great to hear. I think this will be very helpful. Just make a little note on the main page saying that if nothing else is specified the codes are under public domain or something like that.
Title: Re: Settings parser
Post by: Foaly on January 05, 2014, 07:02:03 pm
Hello everybody!
I have been using the SettingsParser class from the wiki quiet a lot, especially for small projects (for settings or to keep some data between application runs).
I have modified/extended/improved the class a lot over time. The functionallity pretty much stayed the same, but I removed memory leaks, improved the error handeling, removed code duplication, changed the way files are loaded and lots of other stuff. I found some time today to clean it up and upload the code.
I invite everybody to take a look at it (https://github.com/SFML/SFML/wiki/Source:-Settings-Parser) and give me some feedback.
I hope find this class as useful as I did.
Title: Re: Settings parser
Post by: Jove on July 27, 2014, 10:36:43 am
This is indeed useful, thanks for updating it.

I have however noticed that when I 'set' a value in the file and later the destructor calls write(), it adds a line to the file. These tend to build up after a while.
Title: Re: Settings parser
Post by: Foaly on July 28, 2014, 10:41:41 am
I am glad you find it helpful! :)
I noticed that bug as well. I did a quick check and the write loop looks fine. There are no empty lines being written, but still one empty line appears in the file. It might be something releated to line endings, but sadly I don't really have time to investigate right now...
What system are you on?
Title: Re: Settings parser
Post by: Laurent on July 28, 2014, 10:56:00 am
This may be caused by the reading loop:

    while(!in.eof())
    {
        std::getline(in, line);
        if(line.size() > 0 && line[0] != '#')
        {
             ...
        }
        else
        {
            param = line;
            value = "";
        }
        m_data.push_back(make_pair(param, value));
        m_size++;
    }

!eof() is not a valid stop condition for reading file content. My guess is that you have one unwanted iteration, and end up adding an empty pair every time you read the file. The reason is that eof() will return true only after you try to read past the end of the file. But if you are at the end, not after the end, it will still return false. In fact this function is only meant to be used after the stream has switched to invalid state, to query the reason.

You can google it to find more explanations and more correct ways of stopping read loops (quickly: use while (std::getline(...)) and everything should be fine).
Title: Re: Settings parser
Post by: Foaly on July 28, 2014, 07:46:53 pm
Ah that makes sense!
And indeed it fixed the problem. It seems like this was a part from the old code, that I forgot to refactor. Now that I look at it using !in.eof() makes no sense at all. I changed it to std::getline(...).
I wrote a little update for the class (fixed a complier warning and did some other stuff). It's on the wikipage :)
Title: Re: Settings parser
Post by: Jesper Juhl on July 28, 2014, 07:54:58 pm
This looks nice and useful.  :)
Although for my own work I prefer libconfig (http://www.hyperrealm.com/libconfig/).
Title: Re: Settings parser
Post by: Foaly on July 29, 2014, 01:35:02 pm
Thanks for the link! Looks like an interesting library for projects that are a bit bigger. I will definitly check it out!
Title: Re: Settings parser
Post by: Nexus on July 29, 2014, 02:27:48 pm
Could you update the link in the initial post? :)

By the way, why have you added const to every parameter? This does not improve const-correctness, it just adds information that is useless to the caller and therefore makes the function interface more difficult to understand. See here (http://en.sfml-dev.org/forums/index.php?topic=2767.0) and here (http://en.sfml-dev.org/forums/index.php?topic=14908.msg105005#msg105005) for detailed explanation.
Title: Re: Settings parser
Post by: select_this on July 29, 2014, 03:33:10 pm
By the way, why have you added const to every parameter? This does not improve const-correctness, it just adds information that is useless to the caller and therefore makes the function interface more difficult to understand.

It's probably worth clarifying that this applies to the copied second args (const bool value, const char value etc.), not the string references.
Title: Re: Settings parser
Post by: Jesper Juhl on July 29, 2014, 05:55:24 pm
By the way, why have you added const to every parameter? This does not improve const-correctness, it just adds information that is useless to the caller and therefore makes the function interface more difficult to understand.

I agree that it's useless as part of the interface, but it can sometimes be useful to do it in the implementation.
Like so:
void someFunc(int i);

void someFunc(const int i)
{
... do stuff involving 'i' ...
}
If I don't intend to modify 'i' then I want the compiler to help me and tell me if I do, so I want 'i' to be 'const'. But of course it is pointless for users of the function since I take 'i' by value, so the declaration in the header should of course omit the 'const'.
Title: Re: Settings parser
Post by: Nexus on July 29, 2014, 06:24:48 pm
I agree... If you read the links I've posted, you'll see that both of your objections are addressed there ;)

Quote from: Nexus
Declaring parameters const is meaningful for references and pointers, but on the top-level (like const sf::Vector param) it clutters the interface with irrelevant information.
Quote from: Nexus
One can follow your suggestions in the implementation (function definition), but never in the interface (declaration).
Title: Re: Settings parser
Post by: select_this on July 29, 2014, 06:54:33 pm
Mine wasn't an objection, mine was a 'let's clarify before someone reads this thread and doesn't bother to follow the links' ;)
Title: Re: Settings parser
Post by: Jesper Juhl on July 29, 2014, 07:02:59 pm
No objections here either. Just wanted to point out some details  :D
Title: Re: Settings parser
Post by: Foaly on July 30, 2014, 11:59:07 am
Aah damn! I knew something felt odd, while I was doing this... But it was early in the morning and I thought I simply forgot the const. Didn't think there for a minute :D
Thanks for pointing this out. And also thanks for the links! I didn't know you could make the parameters in the implementation const but not in the interface. Very interessting.
Anyway I removed the const from the interface.
Title: Re: Settings parser
Post by: SeriousITGuy on August 04, 2014, 09:53:25 am
Just stumbled across the settings parser on the wiki, and I have a question regarding the data structure you save the data in. Why do you use a std:vector<std::pair<std::string, std::string>>  ?
With a vector of pairs you can enter duplicate settings, and your code then will always only select the value with the lower index-key in your vector. The second value with the same key (but higher index in your vector) will never get selected (and shouldn't be there in the first place in my opinion).

That's a little design problem and I think this should be addressed. If a std::map<std::string, std::string> is used instead of a vector, only one instance of a key can live inside the map, which gets overwritten if the same key is present in the text file (which could also be eliminated easily using a map).

Also maybe some checking if the given text file yields a valid format maybe benefical, if the user manually edits the text-file (which developers often do for unit testing). Some points from the top of my head:
- Left- and right-trimming the lines for whitespace before parsing them
- converting the key to be always lower (or upper) case, so that it's really unique (so case-variants of identical keys can also be detected)

Throwing an exception instead of returning false if the text file cannot be read may also be better, a bool-return can be ignored, a thrown exception not ;)

Just some improvement thoughts to make the already good implementation more robust.
Cheers!


OT-Question:
I agree that it's useless as part of the interface, but it can sometimes be useful to do it in the implementation.
Like so:
void someFunc(int i);

void someFunc(const int i)
{
... do stuff involving 'i' ...
}


How do you do this? If I declare a function like you without the 'const' and then define it with 'const', I get a C2511: Overloaded member function not found in '...'
I see the benefit of it, but can't implement it. Confused here  :o
Title: Re: Settings parser
Post by: Nexus on August 04, 2014, 10:35:01 am
Quote
How do you do this? If I declare a function like you without the 'const' and then define it with 'const', I get a C2511: Overloaded member function not found in '...'
What compiler? Do you have a minimal complete example?

According to the C++ standard, top-level CV qualifiers do not contribute to the parameter type. That is,
void function(int i);
void function(const int i);
are the same signatures. This, of course, does not apply to const-references, const-pointers or a memberfunction const at the end referring to this.
Title: Re: Settings parser
Post by: SeriousITGuy on August 04, 2014, 10:48:06 am
This, of course, does not apply to const-references

Haha, that's it, was working with references and did not notice its pointed towards copy-values. Thanks again ;)
Title: Re: Settings parser
Post by: Foaly on August 05, 2014, 11:16:40 am
Thanks! Those are really good point. But since I am on vacation, I can't investigate your suggestions… I will look into it when I get back  :)
Title: Re: Settings parser
Post by: SeriousITGuy on August 27, 2014, 08:13:12 pm
I took the liberty of coding something myself with your code as a starting point, and worked in my thoughts.
Here is my version of a very simple config file parser https://github.com/SeriousITGuy/ConfigParser

Title: Re: Settings parser
Post by: Jesper Juhl on August 27, 2014, 08:36:07 pm
For simple config files I personally just use libconfig (http://www.hyperrealm.com/libconfig/). It has a nice C++ API, does all I need (and more than I could be bothered to implement on my own), is free, had a permissive license and works very well.
Title: Re: Settings parser
Post by: SeriousITGuy on August 28, 2014, 09:45:39 am
Yeah, I know libconfig, already worked with it, it is indeed very good and just does the job.
I wrote my own 'very simple' parser just for the sake of simplicity, and for use it in my future projects because only thing I need is a simple key=value parser. If one day I need something elaborate like the compound config settings libconfig is able handling of, I will choose libconfig for sure ;)

Just wanted to share with the SFML-Comm because it shared so much with me.  ;)
Title: Re: Settings parser
Post by: Jesper Juhl on August 28, 2014, 05:04:34 pm
Thank you for sharing  :)
Title: Re: Settings parser
Post by: Foaly on September 18, 2014, 11:39:21 am
Hello everybody!
Like I said I was away for a bit, but I found some time to look at the code again.
I thought about the suggestion that Jesper Juhl made (using a std::map instead of a std::vector, to ensure unique keys), but since this class also supports changing the values and writing them back to the file, the order of insertion is important and needs to be preserved. That's why I will stick with the std::vector. I updated the documentation to clarify that keys have to be unique.
I also refactored the read() method to be more robust handling whitespace and I rearranged some includes.
Title: Re: Settings parser
Post by: dabbertorres on September 18, 2014, 08:00:14 pm
You could maybe use a map AND a vector. Have the map contain all the keys and values, and have the vector store all the keys, thereby keeping order. That way you get the benefits of both (order and unique keys), and I don't think it would make it that much more complex.
Title: Settings parser
Post by: p5ych0 on September 18, 2014, 09:42:26 pm
std::unordered_map?
Title: AW: Re: Settings parser
Post by: eXpl0it3r on September 18, 2014, 09:59:27 pm
std::unordered_map?
It doesn't sort by keys, but it also doesn't retain an order. The order you'd get when iterating over a std::unordered_map depends (afaik) on the used hash function.
Title: Re: Settings parser
Post by: Nexus on September 20, 2014, 06:43:19 pm
That's why I will stick with the std::vector. I updated the documentation to clarify that keys have to be unique.
I would also check this precondition in the code with assertions.
assert(std::find(v.begin(), v.end(), value) == v.end());

When deciding about vector/map/unordered_map, choose the most simple or standard one to use (usually std::vector) unless you have specific reasons to deviate. Maps and even more hash maps can only play their advantages in the presence of many elements (100 are not many).

And please add a link to your initial post ;)
Title: Re: Settings parser
Post by: Foaly on September 21, 2014, 03:26:38 pm
Alrighty I gave the class another overhaul. Now only the data nessessary is kept in memory. Keys are now unique and will be overwritten if used more than once. A std::map is used to ensure that and also because it is the easiest to find the keys. Writing modified data back to the file is still supported.
Also strings containing whitespace are now supported and there were quiet a few bug fixes :)
I created a git repo with the code here (https://github.com/Foaly/SettingsParser) and I update the code on the wiki page.

edit: @Nexus: what kind of link do you want me to add in the first post? A link to the wiki page is already there.
Title: Re: Settings parser
Post by: Nexus on September 21, 2014, 06:04:47 pm
Oh sorry, my bad. I just scrolled up and looked at the first post, not realizing it wasn't the first page ;)

Looking at your code, you seem to do a lot of manual work to extract strings, split and parse them. std::ifstream::operator>> is sometimes not flexible enough to do everything, but it could still simplify a lot if you use formatted I/O capabilities of the standard library.

I once wrote a simple parser for a text file where every line is either empty or contains a key-value pair according to the format key = value, with extremely few code:
std::ifstream file(filename);
std::map<std::string, double> map;

std::string key;
std::string assignment;
double value;
while (file >> key >> assignment >> value)
        map[key] = value;
It will be more complex to handle comments and special cases, but maybe this can still help as an inspiration.
Title: Re: Settings parser
Post by: Foaly on September 29, 2014, 12:30:30 am
Wow that is indeed very few lines for a simple key value parser. Thanks for sharing the code.
I don't think it will work for my case though, because I want to support strings containing whitespace as a value type. And according to the doc (http://www.cplusplus.com/reference/string/string/operator%3E%3E/) the >> operator separates at whitespace, meaning I would only get the first word. I don't see a way to change that, so there seems to be no other way but std::getline().
Title: Re: Settings parser
Post by: Nexus on September 29, 2014, 01:40:18 pm
You can use the std::noskipws (http://en.cppreference.com/w/cpp/io/manip/skipws) manipulator :)
Title: Re: Settings parser
Post by: Foaly on October 05, 2014, 02:44:12 am
I investigated a little bit and unless I am missing something, I don't think this can help me. From what I understand std::noskipws only effects leading whitespace. But what I would need is to change the seperator for the >> operator. So I can parse something like that
Code: [Select]
key = some value containing spaceBut I don't think that's possible, is it?
Title: Re: Settings parser
Post by: Laurent on October 05, 2014, 09:10:27 am
You already gave the solution: std::getline. What's wrong with it?
Title: Re: Settings parser
Post by: Foaly on October 05, 2014, 02:35:18 pm
There is nothing wrong with std::getline(), that's what I am using right now.
But Nexus suggested that he did something a little more elegant with >>. That's why I asked if it's possible to extract multiple words containing whitespace into one string.
But I guess that's not possible, so I'll stick with getline()
Title: Re: Settings parser
Post by: Laurent on October 05, 2014, 10:07:32 pm
Why would operator >> be more elegant than std::getline? The code logic can be the exact same, just with one function instead of the other.
Title: Re: Settings parser
Post by: SeriousITGuy on November 14, 2014, 11:31:51 am
Because one would need less code to actually parse the line and store the key/value-pairs, like Nexus posted. But this comes with the limitation that you can actually not do any error or syntax checking in the text file, that's the reason why I use std::getline() and then a parse-function with 40 LOCs:

(click to show/hide)

This way it is still a simple parse-function (which could be expanded upon) but with a robust use.

@Foaly: You could make your code more readable and less typing-error prone if you would use auto, e.g.
auto it = m_data.find(key);
instead of
std::map<std::string, std::string>::const_iterator it = m_data.find(key);

These dumb iterator-types are actually one of the best reasons why auto even exists. ;)
Title: Re: Settings parser
Post by: Foaly on December 06, 2014, 11:26:42 pm
Hi there!
The settings parser class has gone through some changes recently. I finally had some time to wrap it all up nicely and update the wiki page.
The class uses templates now and features things like reading/writing values from/to a vector. Parsing has been moved to a seperate function and is now more robust. Lots of small things (like using auto) have changed. For a full changelog just refer to the commit history of the repo (https://github.com/Foaly/SettingsParser).
All the things mentioned in this thread should be fixed. So check it out and let me know what you think! :)
Title: Re: Settings parser
Post by: Laurent on December 06, 2014, 11:56:49 pm
A few comments:

1. The string <--> T conversion functions should definitely not be part of the public header. They should be in their own private header, included in the .cpp only.

2. Why did you split the load and save functions in two parts (one tiny and one that does all the job)?

3. Is the locale member useful? Isn't the one argument version of std::isspace using the default locale anyway?
Title: Re: Settings parser
Post by: wintertime on December 07, 2014, 11:42:35 am
You don't initialize all members in the constructor. That means the locale gets default-constructed from the currently active global locale that someone could have set to anything: http://en.cppreference.com/w/cpp/locale/locale/locale . Then the stream could use a different locale (internally copied from the global locale at time of its construction) than your later calls to isspace, if it got changed inbetween.
It would be best to not use any function that can depend on a locale (especially when parsing files that should never depend on one), but if you can't help it explicitely construct a "C" locale and use it for everything.

I also don't like how you read and parse the file again on a therefore mislabeled write method, possibly using a different locale and if the file got changed meanwhile you mix in some possibly old settings.
There I would just read the whole file in binary mode into memory with a single read call to the stream inside your read method, do in memory parsing and keep all data for writing it later without reading it again.
Then you could also have a save method taking a filename to make it also useful for the case of writing a copy of the settings somewhere else, to prevent partial overwriting on errors or have a backup or construct a new settings file without loading it first.
Title: Re: Settings parser
Post by: underww on December 15, 2014, 09:30:18 am
You should handle an exception about a line with only white spaces in the parseLine function.