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.

Sign up for GitHub

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

Jump to bottom

ini_parser: Remove inline comment parsing on ini handler for values #4175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ini_parser: Remove inline comment parsing on ini handler for values #4175

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/inih/ini.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,6 @@ int ini_parse_stream(ini_reader reader, void* stream, ini_handler handler,
*end = '\0';
name = rstrip(start);
value = end + 1;
#if INI_ALLOW_INLINE_COMMENTS
Copy link
Contributor

@nmschulte nmschulte Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/prusa3d/Prusa-Firmware-Buddy/blob/master/lib/inih/README.md#compile-time-options

Inline comments: By default, inih allows inline comments with the ; character. To disable, add -DINI_ALLOW_INLINE_COMMENTS=0. You can also specify which character(s) start an inline comment using INI_INLINE_COMMENT_PREFIXES.

Start-of-line comments: By default, inih allows both ; and # to start a comment at the beginning of a line. You can override this by changing INI_START_COMMENT_PREFIXES.

Did you consider simply tweaking either or both of these options, vs eliding this single dependent block of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to remove inline comments. Instead we just want to remove inline comments where inline comments shouldn't happen : On parsing values. That's why only this define is removed, but INI_ALLOW_INLINE_COMMENTS stays defined.

Copy link
Contributor

@nmschulte nmschulte Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, sounds like a bug in inih then? Or, is this an effect of the quoted string modification? I think it's not, given the comments from the issue this addresses, but unsure.

Perhaps this is fixed in more recent inih? https://github.com/benhoyt/inih/commits/master/ini.c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, it isn't fixed in the latest master, it's an issue by design of the parser. It would require to be rewritten and it would add more complexity. However this is only a temporary fix to solve current user issues until the inih is completely removed and replaced by a different parser library.

end = find_chars_or_comment(value, NULL);
if (*end)
*end = '\0';
#endif
value = lskip(value);
rstrip(value);
value = unquote(value);
Expand Down