Skip to content

Conversation

jwnimmer-tri
Copy link

@jwnimmer-tri jwnimmer-tri commented Aug 29, 2025

FYI For reviewers:

  • I've configured Reviewable now to only show files that are relevant to Drake. Everything else is binned in a "vendored" grouping.
  • r1 contains the merge commit with upstream.
  • r2+ are adjustments to (hopefully) keep the updates from upstream aligned with Drake conventions.
  • For a preview click here. Note that this live preview lacks a little supplemental styling (link icon, css style details, etc.) but is reasonable for reviewing text. See https://drake.mit.edu/documentation_instructions.html if you'd like a 100% authentic preview.

Highlights:

  • cpplint is deleted
  • new license
  • new section on concepts
  • new section on consteval & related
  • modules verboten
  • prefer converting a class using std::string(some_cord) instead of static_cast<std::string>(some_cord).

This change is Reviewable

FaresSalem and others added 30 commits October 6, 2020 17:15
- Include friend types in class declaration order guidance.
- Include previously omitted text (due to mismatched tags) about
  preferring int16_t over short, et cetera.
- Function declarations:
  - Updated guidance for what comments should cover.
  - Add a C++ attribute example.
PiperOrigin-RevId: 480954960
There is no Common Lisp operator named PRINT-UNPRINTABLE-OBJECT. Given the context, it is certain that PRINT-UNREADABLE-OBJECT (www.lispworks.com/documentation/lw51/CLHS/Body/m_pr_unr.htm) was meant instead.
Update ts styleguide with latest google version
PiperOrigin-RevId: 489165518
PiperOrigin-RevId: 489255983
Go: Fix a logic error in an example.
s/Vimscript/Vim script/
PiperOrigin-RevId: 489430034
PiperOrigin-RevId: 489956630
Add link to the Go guide from README.md.
PiperOrigin-RevId: 493761232
PiperOrigin-RevId: 497970050
pylint triggered:

pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.StandardError' ?) instead.
pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead.
pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.BaseException' ?) instead.

Suggestion works out.
PiperOrigin-RevId: 510217476
- Encourage use of the `internal` namespace to document parts of an API
  that are not public.
- Create a separate section for `switch` statements.
- Require a project-specific prefix for macros.
- Reorganize guidance for formatting conditional statements.
- Other miscellaneous wording and formatting fixes.
PiperOrigin-RevId: 522150370
Copy link
Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

@jwnimmer-tri reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),sherm1(platform) (waiting on @ggould-tri, @rpoyner-tri, @sammy-tri, @SeanCurtis-TRI, and @sherm1)

Copy link

@rpoyner-tri rpoyner-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:

@rpoyner-tri reviewed 44 of 44 files at r1, 1 of 2 files at r2, 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),sherm1(platform) (waiting on @ggould-tri, @sammy-tri, @SeanCurtis-TRI, and @sherm1)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

This is new for us, I believe:

When explicitly converting to a class type, use a function-style cast;
e.g., prefer std::string(some_cord) to
static_cast<std::string>(some_cord).

Checkpoint for r1

@sherm1 reviewed 44 of 44 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),sherm1(platform) (waiting on @ggould-tri, @sammy-tri, and @SeanCurtis-TRI)


cppguide.html line 5176 at r1 (raw file):

are mixed case with the first letter of each word capitalized
("<a href="https://en.wikipedia.org/wiki/Camel_case">camelCase</a>" or
"<a href="https://en.wiktionary.org/wiki/Pascal_case">PascalCase</a>").

BTW the above appears to approve of snake_case, PascalCase, and camelCase. I don't think we ever permit camelCase (initial lower case)?

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

:lgtm: with one question below. Also, we should announce the new conversion style noted above. I like it!

@sherm1 reviewed 1 of 2 files at r2, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform) (waiting on @ggould-tri, @jwnimmer-tri, @sammy-tri, and @SeanCurtis-TRI)

Copy link
Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

we should announce the new conversion style noted above.

For starters, I added it to the summary atop this PR.

@jwnimmer-tri reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform) (waiting on @ggould-tri, @sammy-tri, and @SeanCurtis-TRI)


cppguide.html line 5176 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW the above appears to approve of snake_case, PascalCase, and camelCase. I don't think we ever permit camelCase (initial lower case)?

I think its point was to give names to the various styles in the world, without endorsement. Anyway, I pushed a clarification.

Copy link

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

@rpoyner-tri reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform) (waiting on @ggould-tri, @sammy-tri, and @SeanCurtis-TRI)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

@sherm1 reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform) (waiting on @ggould-tri, @sammy-tri, and @SeanCurtis-TRI)

Copy link

@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: With one comment.

@SeanCurtis-TRI reviewed 11 of 44 files at r1, 1 of 2 files at r2, 1 of 1 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees ggould-tri(platform),sammy-tri(platform) (waiting on @ggould-tri and @sammy-tri)


cppguide.html line 524 at r1 (raw file):

        <li>and the function is non-virtual,</li>
        <li>and the function is not recursive,</li>
        <li>and the function is "small" (~10 lines).</li>

BTW The new google documentation now calls out this size limit. We probably don't need to echo it?

Copy link
Author

@jwnimmer-tri jwnimmer-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: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform) (waiting on @ggould-tri and @sammy-tri)


cppguide.html line 524 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The new google documentation now calls out this size limit. We probably don't need to echo it?

So did the old upstream text (in a slightly different place). But in both cases, the upstream wording is more of a suggestion than a rule; here, we specifically strengthen it into a hard requirement.

Copy link

@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: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform) (waiting on @ggould-tri and @sammy-tri)


cppguide.html line 524 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

So did the old upstream text (in a slightly different place). But in both cases, the upstream wording is more of a suggestion than a rule; here, we specifically strengthen it into a hard requirement.

Sounds good.

@jwnimmer-tri
Copy link
Author

Gentle reminder for @ggould-tri and @sammy-tri -- comments are due by next Monday, please.

Copy link

@sammy-tri sammy-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:

@sammy-tri reviewed 1 of 1 files at r6.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri)

Copy link
Collaborator

@ggould-tri ggould-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:; sorry, thought I'd already punched this one.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),sherm1(platform),xuchen-han (waiting on @jwnimmer-tri)

@rpoyner-tri rpoyner-tri merged commit 63d2e8e into RobotLocomotion:main Sep 10, 2025
1 check passed
@jwnimmer-tri jwnimmer-tri deleted the sync-upstream-2025-08 branch September 10, 2025 18:51
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.