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

Update cpp style to match new cpplint target #22722

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Aiden2244
Copy link
Contributor

@Aiden2244 Aiden2244 commented Mar 7, 2025

Towards #22646. This PR introduces C++ style changes to pass cpplint tests with the new upstream target.


This change is Reviewable

Aiden McCormack and others added 12 commits February 26, 2025 11:47
commit 2235825
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 22:40:54 2025 -0500

    fixed comma spacing errors in macos.py

commit 44251d8
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:52:00 2025 -0500

    added blank line in .gitignore

commit 5c8408e
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:50:52 2025 -0500

    removed env variables and hopefully escaped git hell

commit 006d4ee
Merge: ec68da8 8859802
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:46:39 2025 -0500

    Merge branch 'master' into bug-brew-no-lock

    modified local gitignore, reverted change on local master branch

commit ec68da8
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:43:15 2025 -0500

    Revert "removed references to brew lockfiles"

    This reverts commit 14ac2d4.

commit e774794
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:38:00 2025 -0500

    modified gitignore

commit e5789ad
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:31:31 2025 -0500

    removed irrelevant changes

commit 14ac2d4
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:29:17 2025 -0500

    removed references to brew lockfiles

commit a6da02a
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:27:19 2025 -0500

    addressed an instance of '--no-lock' in python build script

commit ebc06c7
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:17:46 2025 -0500

    removed venv from gitignore

commit 0d0f820
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 15:10:01 2025 -0500

    removed --no-lock from source distribution build script

commit 0ba270e
Author: Aiden2244 <github.quicken590@pasmail.net>
Date:   Tue Mar 4 14:47:21 2025 -0500

    added homebrew env variable and removed deprecated flag

Fixed brew lockfile issues
merge latest commits into master
@Aiden2244
Copy link
Contributor Author

+@jwnimmer-tri +@tyler-yankee

Copy link
Collaborator

@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: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @Aiden2244)


common/schema/stochastic.cc line 110 at r1 (raw file):

  }
  double sum = 0;
  for (double value : values) {

nit Is this for whitespace/newline? If yes, then take note that I marked this as "(Not sure, I'll need to look more carefully, later.)" in my discussion on the other PR, so we should not be changing anything to solve it yet.

Ditto for all of the other newline changes.


common/identifier.h line 197 at r1 (raw file):

   */
  template <typename HashAlgorithm>
  // NOLINTNEXTLINE (runtime/references)

nit Pervasively in the whole PR -- no space between NOLINENEXTLINE and the open-paren.

@Aiden2244
Copy link
Contributor Author

nit Pervasively in the whole PR -- no space between NOLINENEXTLINE and the open-paren.

I just uploaded a number of drafts to the discussion detailing my observations. As it turns out, the new regex parser for NOLINT in the upstream does not detect the comment and ignore the line unless there is a space between the open paren and the word. The errors I was encountering would not be suppressed otherwise.

As for the newline revisions, I can revert those no problem. Will do that now.

Copy link
Contributor Author

@Aiden2244 Aiden2244 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, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes


common/identifier.h line 197 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Pervasively in the whole PR -- no space between NOLINENEXTLINE and the open-paren.

Forgive me, still figuring out the UI for Reviewable. I just posted a top-level comment about this particular issue.

In that discussion I mention an observation I made about the updated regex parsing schema. Here is that for reference:

===========

Interesting... there's a slight difference in the regex being tested against when parsing lines for NOLINT

old: matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line)
new: matched = re.search(r'\bNOLINT(NEXTLINE|BEGIN|END)?\b(\([^)]+\))?', raw_line)

The upshot is that the text '// NOLINT(...' does not get detected by the new parser but '// NOLINT (...' with a space between the parentheses does. That was all that was needed for the linter to detect those changes.

...

I noticed a similar parsing issue here as well: // NOLINTNEXTLINE (runtime/references) Per hash_append convention. does not trigger any errors but // NOLINTNEXTLINE(runtime/references) Per hash_append convention. does not get picked up by the new cpplint target...

Copy link
Contributor Author

@Aiden2244 Aiden2244 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, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @Aiden2244)


common/schema/stochastic.cc line 110 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Is this for whitespace/newline? If yes, then take note that I marked this as "(Not sure, I'll need to look more carefully, later.)" in my discussion on the other PR, so we should not be changing anything to solve it yet.

Ditto for all of the other newline changes.

Will do, reverting now.

Copy link
Contributor Author

@Aiden2244 Aiden2244 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: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @Aiden2244)


common/identifier.h line 197 at r1 (raw file):

Previously, Aiden2244 (Aiden McCormack) wrote…

Forgive me, still figuring out the UI for Reviewable. I just posted a top-level comment about this particular issue.

In that discussion I mention an observation I made about the updated regex parsing schema. Here is that for reference:

===========

Interesting... there's a slight difference in the regex being tested against when parsing lines for NOLINT

old: matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line)
new: matched = re.search(r'\bNOLINT(NEXTLINE|BEGIN|END)?\b(\([^)]+\))?', raw_line)

The upshot is that the text '// NOLINT(...' does not get detected by the new parser but '// NOLINT (...' with a space between the parentheses does. That was all that was needed for the linter to detect those changes.

...

I noticed a similar parsing issue here as well: // NOLINTNEXTLINE (runtime/references) Per hash_append convention. does not trigger any errors but // NOLINTNEXTLINE(runtime/references) Per hash_append convention. does not get picked up by the new cpplint target...

Got your message on the other PR, whitespaces are reverted.

Copy link
Collaborator

@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.

Reviewed 4 of 32 files at r1, 7 of 7 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @Aiden2244)


common/type_safe_index.h line 492 at r3 (raw file):

  /// application).
  template <typename HashAlgorithm>
  // NOLINTNEXTLINE(runtime/references)

nit Missing the rest of the explanation comment here.


common/identifier.h line 197 at r3 (raw file):

   */
  template <typename HashAlgorithm>
  // NOLINTNEXTLINE(runtime/references)

nit Missing the rest of the explanation comment here.

(Note that for trivial review discussions like this one where the fix is obvious, it's okay to click "Resolve" to close out the thread once the fix has been pushed. You don't necessarily need to write a reply, though if there's something to say of course feel free.)


solvers/solver_id.h line 38 at r3 (raw file):

  /** Implements the @ref hash_append concept. */
  template <class HashAlgorithm>
  // NOLINTNEXTLINE(runtime/references)

nit Missing the rest of the explanation comment here.


common/hash.h line 80 at r3 (raw file):

std::enable_if_t<std::is_integral_v<T>>
hash_append(
// NOLINTNEXTLINE(runtime/references) Per hash_append convention.

nit In cases like this where the NOLINTNEXLINE is inside the function declaration, ideally we would align it with the argument list (i.e., four more spaces) instead of the function name. Are the linters okay with that alternative spelling?

hash_append(
    // NOLINTNEXTLINE(runtime/references) Per hash_append convention.
    HashAlgorithm& hasher, const T& item) noexcept {

Ditto for the 3x more times it happens later in this file.


multibody/mpm/sparse_grid.cc line 3 at r2 (raw file):

#include "sparse_grid.h"

#include <utility>

nit We do use <utility> for std::pair, below. Why remove it? (Maybe this was a git merge conflict snafu.)


solvers/binding.h line 122 at r3 (raw file):

  /** Implements the @ref hash_append concept. */
  template <class HashAlgorithm>
  // NOLINTNEXTLINE(runtime/references)

nit Missing the rest of the explanation comment here.

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label Mar 8, 2025
Copy link
Collaborator

@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: 7 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)


multibody/mpm/particle_sorter.h line 218 at r3 (raw file):

          spgrid.CoordinateToOffset(base_node[0], base_node[1], base_node[2]);
      /* Confirm the data bits of the base node offset are all zero. */
      DRAKE_ASSERT((base_node_offsets_[p] &

nit See https://drake.mit.edu/styleguide/cppguide.html#Casting.

The recommended spelling is uint64_t{1}, not static_cast. I guess the only problem with the old code was using parens instead of curlies.

(Ditto for the few other cast-related changes in this PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants