Fix or suppress lint in anticipation of cpplint upgrade

[Aiden2244]

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

---

This change is 

[Aiden2244]

+@jwnimmer-tri +@tyler-yankee

[jwnimmer-tri]

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]

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.

[Aiden2244]

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

[Aiden2244]

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.

[Aiden2244]

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.

[jwnimmer-tri]

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]

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

[tyler-yankee]

Reviewed 7 of 7 files at r2, 14 of 21 files at r3, all commit messages.
Reviewable status: 9 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)

---

common/symbolic/chebyshev_polynomial.h line 103 at r3 (raw file):

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

nit Missing explanation here

---

geometry/render_gl/internal_buffer_dim.h line 28 at r3 (raw file):

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

nit Missing explanation here

[Aiden2244]

Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

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.

As far as I can tell, the linters from the upstream are ok with the indentation of these comments, but I will have to double check. On this branch targeting the legacy cpplint fork, the checks pass locally with the alternate spelling.

[tyler-yankee]

Reviewed 1 of 32 files at r1, 9 of 10 files at r4, all commit messages.
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) (waiting on @Aiden2244)

---

multibody/mpm/sparse_grid.cc line 1 at r4 (raw file):

#include "drake/multibody/mpm/sparse_grid.h"

Is there a linter-related reason for this change?

[Aiden2244]

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) (waiting on @Aiden2244)

---

multibody/mpm/sparse_grid.cc line 1 at r4 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Is there a linter-related reason for this change?

Fair question, tl;dr answer is yes.

For some additional context, I added this to a discussion on the OTHER branch when it probably would have been best to include it here as well. Last Friday I wrote the following:

======
"""
Also, interesting observation re IWYU: while debugging the remaining style changes, I noticed that there was a remaining IWYU error in drake/multibody/mpm/sparse_grid.cc. Turns out, including the local path (#include sparse_grid.h) causes the IWYU error, but including the path relative to the root of the repo (# include drake/multibody/mpm/sparse_grid.h) does not. Just thought that was interesting.
"""

[tyler-yankee]

multibody/mpm/sparse_grid.cc line 1 at r4 (raw file):

Previously, Aiden2244 (Aiden McCormack) wrote…

Fair question, tl;dr answer is yes.

For some additional context, I added this to a discussion on the OTHER branch when it probably would have been best to include it here as well. Last Friday I wrote the following:

======
"""
Also, interesting observation re IWYU: while debugging the remaining style changes, I noticed that there was a remaining IWYU error in drake/multibody/mpm/sparse_grid.cc. Turns out, including the local path (#include sparse_grid.h) causes the IWYU error, but including the path relative to the root of the repo (# include drake/multibody/mpm/sparse_grid.h) does not. Just thought that was interesting.
"""

Oh, okay, yeah I briefly saw that comment but good to know that it relates here. This error wasn't caught by the previous linter? Do we follow this convention throughout the rest of the code?

[jwnimmer-tri]

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) (waiting on @Aiden2244)

---

multibody/mpm/sparse_grid.cc line 1 at r4 (raw file):

Do we follow this convention throughout the rest of the code?

It's already used everywhere else. This was just a typo that snuck through review.

[Aiden2244]

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) (waiting on @Aiden2244)

---

multibody/mpm/sparse_grid.cc line 1 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Do we follow this convention throughout the rest of the code?

It's already used everywhere else. This was just a typo that snuck through review.

Just confirmed this. I believe the old linter did not catch this change but the upstream definitely did.

[tyler-yankee]

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)

[jwnimmer-tri]

Reviewed 10 of 10 files at r4, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Aiden2244)