Support for semicolon comment delimiter

[teeks99]

INI style configuration files should support the semicolon ';' character, when
it is the first character on the line only, as a comment delimiter. Because
the '#' style comments are not allowed in strict MS INI files, this gives an
opening for files to be compatible in both and still have comments.

[teeks99]

This could, in theory, cause errors in existing files...if there are valid lines/values that start with semi-colons. I suspect there are very close to zero instances where this is the case, because that would be a weird thing to do. More realistically, there are people who have files with what they think are comments, but the lines are actually being processed.

Either way, if we don't want to change the file format in this minor way, we can add another defaulted parameter (defaulting it off) to the parse_config_file interface.

[jeking3]

This seems reasonable to me.

[eyalroz]

1. Why check for a semicolon before, rather than after, trimming whitespace? If we support # comments (which are not integral to the INI format AFAICR), and if we trim whitespace before really processing a line, let's also consider ;blah blah blah to be a comment.
2. The original code uses *s.begin() instead of *s.at(0), I suggest maintaining uniformity in this respect.
3. Given the current choice of when to check for the semicolon, why go through whitespace trimming, instead of continuing to the next loop iteration immediately?

The code I'd put in here would be:

s = trim_ws(s);
if (!s.empty()) {
    if (*s.begin() == ';') { 
        continue; // an INI-style comment line
    }
    /* etc. etc. */

[jjELT]

This has been fixed 4 years ago. Why is this not yet merged into the latest Boost release?

[vprus]

There are outstanding questions above, as you can see.

[eyalroz]

@jjELT : You could "adopt" this PR by answering the questions, and possibly creating a new PR after they've been resolved.

But just to clarify - I don't hold any authority here, it's @vprus' repository... I just commented since I was more interested in PRs here back in 2018.

[jjELT]

Ok, let me see if I understood correctly the open issues.

1. This could, in theory, cause errors in existing files...if there are valid lines/values that start with semi-colons. [...]

2. [...] if we don't want to change the file format in this minor way, we can add another defaulted parameter (defaulting it off) to the parse_config_file interface.

According to Wiki:

Semicolons (;) at the beginning of the line indicate a comment.

So it would actually be wrong to make sure a line starting with a semicolon would be interpreted as a valid setting.
It is therefore not necessary to add an option, to enable semicolons to be interpreted as a comment. It must be interpreted as a comment.

If you read on in the Wiki article, you notice it would make much more sense to optionally interpret the number sign as a comment instead, since by default it should only be interpreted as a pseudo line comment character, hiding a setting by changing it's name.
Since the effect is the same, and boost_options would throw an Unknown setting '#my-hidden-setting' (or similar, unless unkown settings are allowed of course), I would also leave the number sign to be interpreted as a comment.

On stackoverflow.com there is a related thread concerning this topic.

The current implementation

// strip '#' comments and whitespace
if ((n = s.find('#')) != string::npos)
	s = s.substr(0, n);
// if the first character is a ';' line is a comment
if (!s.empty() && ';' == s.at(0))
	s = "";
s = trim_ws(s);

should be just fine.

Am I missing something?

[eyalroz]

Am I missing something?

My questions.

[teeks99]

The documentation I've found says something along the line of "semicolon at the start of the line". I feel like trimming white space before it could potentially cause issues. Though the only use case I can think of is where someone wants a single semicolon as the contents of a value....not super useful, but I want to be careful as people might be depending on weird side effects of semicolons not working as a comment other places in the line.

Point taken on s.begin() instead of s.at(0).

If we don't change it to remove the white space before checking for a comment line, there's no harm in doing it after, as the empty string won't have any.

[eyalroz]

Well, emphasizing again that it's Vladimir's call, not mine. But:

The documentation I've found says something along the line of "semicolon at the start of the line"

Well, there's no official spec, so ok I guess.

If we don't change it to remove the white space before checking for a comment line, there's no harm in doing it after, as the empty string won't have any.

... but it's useless work.

[teeks99]

@eyalroz I've updated, does that look good to you?

[eyalroz]

Can't you just do continue; here?

[eyalroz]

A also want to see a commented-out line which has an assignment, and a check to ensure the value is not assigned.

[eyalroz]

@eyalroz I've updated, does that look good to you?

Other than a couple of nitpicks, yes. But again - you're asking the wrong person :-)

[jjELT]

I am happy to see there is some more activity in this thread.

Unfortunately continuous-integration/appveyor/pr — AppVeyor build failed. Since all other tests passed I am wondering whether AppVeyor really fails because of the changes of this pull request, or is it simply configured wrong?!

@vprus: Do you have any more doubts and suggestions or is this code ready to be merged?

[teeks99]

@eyalroz @vprus I just wanted to ping this thread and see if there were any outstanding issues preventing it from merging?

The appveyor failure above is also present on develop, not introduced in this pull request.

[eyalroz]

@teeks99 : Again, I have no say here, it's @vprus 's repo.