Skip to content

toml: fix parsing arrays with newlines#2575

Merged
stephenberry merged 1 commit into
stephenberry:mainfrom
makekryl:toml-arrays-newlines
May 16, 2026
Merged

toml: fix parsing arrays with newlines#2575
stephenberry merged 1 commit into
stephenberry:mainfrom
makekryl:toml-arrays-newlines

Conversation

@makekryl
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +28 to +46
// Skip whitespace, newlines and comments
template <class It, class End>
inline void skip_ws_newlines_and_comments(It&& it, End end) noexcept
{
while (it != end) {
if (*it == ' ' || *it == '\t' || *it == '\n' || *it == '\r') {
++it;
}
else if (*it == '#') {
while (it != end && *it != '\n' && *it != '\r') {
++it;
}
}
else {
break;
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not seem good (hands are itching to merge with skip_ws_and_comments via a bool template parameter), on the other hand this is a simple way to do that without overcomplicating ¯\_(ツ)_/¯

Copy link
Copy Markdown
Contributor Author

@makekryl makekryl May 16, 2026

Choose a reason for hiding this comment

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

I'm also not sure whether it would require a separate benchmark like skip_ws_benchmark

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think a separate function is good and clear to read. No need to add a separate benchmark for this. At some point I'll work on tighter optimization of TOML, but performance isn't as critical typically for TOML usage.

@makekryl makekryl force-pushed the toml-arrays-newlines branch from 990716b to ceec28f Compare May 16, 2026 21:48
@stephenberry
Copy link
Copy Markdown
Owner

I like this! Thanks for the PR, merging it in.

@stephenberry stephenberry merged commit 59b2341 into stephenberry:main May 16, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants