Skip to content

avoid using impl in error_printer example #268

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.

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

Merged
merged 1 commit into from
May 7, 2025

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Apr 1, 2025

What does this change do?

Replaced access to toml::impl::control_char_escapes with manual handling of control characters, in print_string.

Would ease a possible future replacement of #include <toml++/toml.hpp> with import tomplplusplus (because toml::impl is not exported).

Is it related to an existing bug report or feature request?

  • I've read CONTRIBUTING.md
  • I've rebased my changes against the current HEAD of origin/master (if necessary)
  • I've added new test cases to verify my change
  • I've regenerated toml.hpp (how-to)
  • I've updated any affected documentation
  • I've rebuilt and run the tests with at least one of:
    • Clang 8 or higher
    • GCC 8 or higher
    • MSVC 19.20 (Visual Studio 2019) or higher
  • I've added my name to the list of contributors in README.md

@N-Dekker N-Dekker force-pushed the avoid-impl-in-error_printer-example branch from 9ecb963 to ac1644a Compare April 1, 2025 10:54
Comment on lines +117 to +123
: (c == '\a') ? "\\a"sv
: (c == '\b') ? "\\b"sv
: (c == '\f') ? "\\f"sv
: (c == '\n') ? "\\n"sv
: (c == '\r') ? "\\r"sv
: (c == '\t') ? "\\t"sv
: (c == '\v') ? "\\v"sv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Two simple escape sequences, \a (0x07) and \v (0x0B), are not in toml::impl::control_char_escapes:

inline constexpr std::string_view control_char_escapes[] =
{
"\\u0000"sv,
"\\u0001"sv,
"\\u0002"sv,
"\\u0003"sv,
"\\u0004"sv,
"\\u0005"sv,
"\\u0006"sv,
"\\u0007"sv,
"\\b"sv,
"\\t"sv,
"\\n"sv,
"\\u000B"sv,
"\\f"sv,
"\\r"sv,

I found them at https://eel.is/c++draft/lex.ccon#nt:simple-escape-sequence-char saying:

simple-escape-sequence-char: one of
  '  "  ?  \ a  b  f  n  r  t  v

Would it be worth adding these two to toml::impl::control_char_escapes as well? Honestly it doesn't matter much to me. Just asking 😃

Copy link
Owner

Choose a reason for hiding this comment

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

They are in there:

  • \u0007
  • \u000B

This list is not used for parsing, so does not need to exhaustively list all possible representations. It's only used during serialization. Each entry in the array is at an index that corresponds to the equivalent codepoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @marzer Just for my understanding, do you mean it's better to use \u0007 and \u000B (rather than \a and \v). Or do you mean that it just doesn't matter? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The print_string function in error_printer example is meant to print a human-readable output, so I guess for print_string, the human-friendly simple-escape-sequences \a and \v may be preferred, whereas for impl::control_char_escapes, \u0007 and \u000B may be preferred. Right?

Copy link
Owner

@marzer marzer Apr 24, 2025

Choose a reason for hiding this comment

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

I mean it doesn't matter, either should work when writing TOML. But you can't add additional values to that impl array as you suggested because the index of the elements matches their content. I don't recall why I chose those representations, was a few years ago now 🤷🏼‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks! By the way, I noticed something weird (?) in the draft of the C++ Standard. Table 10 - Simple escape sequences [tab:lex.ccon.esc] mentions both \a and \v. But there's another table, Table 112 - Mapping of characters to escape sequences [tab:format.escape.sequences], that doesn't mention them at all! Could it be that they accidentally forgot to mention those two, in table 112? It is part of a description of how to construct an escaped string, in section [format.string.escaped]

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, entirely possible. It's a huge document.

@N-Dekker N-Dekker marked this pull request as ready for review April 1, 2025 11:16
Replaced access to `toml::impl::control_char_escapes` with manual handling of control characters, in `print_string`.

Would ease a possible future replacement of `#include <toml++/toml.hpp>` with `import tomplplusplus` (because `toml::impl` is not exported).
@marzer marzer force-pushed the avoid-impl-in-error_printer-example branch from ac1644a to 0921490 Compare May 7, 2025 08:22
@marzer marzer merged commit 2693c6c into marzer:master May 7, 2025
11 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