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.

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

[common] Add fmt_debug_string polyfill #22150

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Nov 11, 2024

Towards #22078.


Sometimes we want to print or log a string. Imagine this function:

void DoFoo(const std::string& arg);

For logging, we might try:

void DoFoo(const std::string& arg) {
  if (condition) {
    drake::log()->warn("Suspicious arg = {}", arg);
  }
  ...
}

That would print e.g.

WARNING Suspicious arg = Hello, world!

However, we'll usually want to print the quoted string value, so that the user can see e.g. if they have training whitespace:

void DoFoo(const std::string& arg) {
  if (condition) {
    drake::log()->warn("Suspicious arg = \"{}\"", arg);
  }
  ...
}

That would print e.g.

WARNING Suspicious arg = "Hello, world!"

That's great! But what if the string itself has double quotes inside of it? The output would be confusing. Or, some strings might contain non-printable characters like 0x08, or awkwardly-printable characters like a tab (\t).

Format has a solution for that:

void DoFoo(const std::string& arg) {
  if (condition) {
    drake::log()->warn("Suspicious arg = {:?}", arg);
  }
  ...
}

The ? presentation type is the "debug string" format, which not only double-quotes the string but also converts non-printable characters into their \xNN hex code, so that the output is a string literal that can be pasted back into either C++ or Python.

Unfortunately, our Jammy copy of fmt is just slightly too old to contain this feature, so we need to polyfill it until we drop jammy.

void DoFoo(const std::string& arg) {
  if (condition) {
    drake::log()->warn("Suspicious arg = {}", fmt_debug_string(arg));
  }
  ...
}

This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: feature This pull request contains a new feature labels Nov 11, 2024
@jwnimmer-tri jwnimmer-tri force-pushed the debug-string-polyfill branch 2 times, most recently from fc23ae1 to 36e41fe Compare November 11, 2024 20:50
@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for feature review, please. Low priority.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

:LGTM:

+@xuchenhan-tri for platform review.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform)


common/fmt.h line 46 at r1 (raw file):

/** Returns `fmt::("{:?}", x)`, i.e, using fmt's "debug string format"; see
the fmt.dev docs for the '?' presentation type for details. We provide this

BTW Is this a widely used abbreviation of which I'm not familiar? The format implies a file or a URL more generally. Or is it simply a typo?

Code quote:

fmt.dev

common/fmt.h line 49 at r1 (raw file):

wrapper because not all of our supported platforms have a new-enough fmt
to rely on it. On platforms with older fmt, we use a Drake re-implementation
of the feature that does NOT handle unicode correctly. */

BTW This gives me a great appreciation of all the work that fmtlib does.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions


common/fmt.h line 46 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Is this a widely used abbreviation of which I'm not familiar? The format implies a file or a URL more generally. Or is it simply a typo?

I think this is meant as a URL. https://fmt.dev/11.0/syntax/ has some more details on the use of '?'

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions


common/fmt.h line 46 at r1 (raw file):

Previously, xuchenhan-tri wrote…

I think this is meant as a URL. https://fmt.dev/11.0/syntax/ has some more details on the use of '?'

Oh, I inferred that as the most logical. But I'd expect either something more explicit (like a link) or more colloquial "the fmt dev docs" -- without the dot. It's the dot that just threw me off as an expression of the concept I've not seen before.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

@jwnimmer-tri jwnimmer-tri merged commit 5de3068 into RobotLocomotion:master Nov 15, 2024
8 of 9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the debug-string-polyfill branch November 15, 2024 00:40
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants