diff --git a/CPPLINT.cfg b/CPPLINT.cfg index 9d756d577669..3a81402c8f0c 100644 --- a/CPPLINT.cfg +++ b/CPPLINT.cfg @@ -12,6 +12,10 @@ set noparent # this warning is irrelevant. filter=-build/c++11 +# Unlike Google, drake embraces . +# https://drake.mit.edu/styleguide/cppguide.html#Other_Features +filter=-build/c++17 + # Drake uses `#pragma once`, not the `#ifndef FOO_H` guard. # https://drake.mit.edu/styleguide/cppguide.html#The__pragma_once_Guard filter=-build/header_guard @@ -39,6 +43,10 @@ filter=-build/include # by the readability/todo rule, which we leave enabled.) filter=-whitespace/todo +# TODO(jwnimmer-tri) This has hundreds of false positives. Seems completely +# broken. With our growing use of clang-format, we probably don't care. +filter=-whitespace/indent_namespace + # TODO(#1805) Fix this. filter=-legal/copyright diff --git a/MODULE.bazel b/MODULE.bazel index ee565a0ee0fd..ab5526ec1cd4 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -191,6 +191,7 @@ use_repo( "common_robotics_utilities_internal", "commons_io", # TODO(jwnimmer-tri) Mark as internal. "conex_internal", + "cpplint_internal", "csdp_internal", "curl_internal", "dm_control_internal", diff --git a/bindings/pydrake/common/text_logging_pybind.cc b/bindings/pydrake/common/text_logging_pybind.cc index 679837e91888..fbfa2ee62cb9 100644 --- a/bindings/pydrake/common/text_logging_pybind.cc +++ b/bindings/pydrake/common/text_logging_pybind.cc @@ -64,7 +64,7 @@ class pylogging_sink final is_configured_.store(true); } - // NOLINTNEXTLINE(build/namespaces) This is how pybind11 wants it. + // NOLINTNEXTLINE(build/namespaces_literals) This is how pybind11 wants it. using namespace pybind11::literals; // Construct the LogRecord. diff --git a/common/hwy_dynamic_impl.h b/common/hwy_dynamic_impl.h index 2f4fba543457..16c6340603a2 100644 --- a/common/hwy_dynamic_impl.h +++ b/common/hwy_dynamic_impl.h @@ -9,7 +9,7 @@ // This file should only ever be included from `*.cc` implementation files, // and we always want its code to be private to that file, so we'll use an // anonymous namespace in a header file, for simplicity. -namespace { // NOLINT(build/namespaces) +namespace { // NOLINT(build/namespaces_headers) /* LateBoundFunction is a small wrapper class around a C-style raw function pointer. (It doesn't support std::function objects.) diff --git a/common/overloaded.h b/common/overloaded.h index 93c70934b8d6..daa73e8704a8 100644 --- a/common/overloaded.h +++ b/common/overloaded.h @@ -27,7 +27,7 @@ use the variant visit pattern, i.e. // We put this in the anonymous namespace to ensure that it will not generate // linkable symbols for every specialization in every library that uses it. -namespace { // NOLINT(build/namespaces) +namespace { // NOLINT(build/namespaces_headers) // Boilerplate for `std::visit`; see // https://en.cppreference.com/w/cpp/utility/variant/visit diff --git a/doc/_pages/clion.md b/doc/_pages/clion.md index 5aea19028360..f6b2567220e1 100644 --- a/doc/_pages/clion.md +++ b/doc/_pages/clion.md @@ -244,7 +244,7 @@ following values for the fields: * **Name:** ``Cpplint File`` * **Description:** ``Apply cpplint to the current file`` * **Program:** ``bazel`` -* **Arguments:** ``run @styleguide//:cpplint -- --output=eclipse +* **Arguments:** ``run @cpplint_internal//:cpplint -- --output=eclipse $FilePath$`` * **Working directory:** ``$Projectpath$`` * **Advanced Options:** Confirm ``Open console for tool output`` is checked @@ -304,7 +304,7 @@ Change the following fields in the instructions given above: Building the google styleguide lint tool: -``bazel build @styleguide//:cpplint`` +``bazel build @cpplint_internal//:cpplint`` **Drake style addenda** diff --git a/examples/pendulum/pendulum_parameters_derivatives.cc b/examples/pendulum/pendulum_parameters_derivatives.cc index 86eaa0640e0c..b26e2c230c89 100644 --- a/examples/pendulum/pendulum_parameters_derivatives.cc +++ b/examples/pendulum/pendulum_parameters_derivatives.cc @@ -47,7 +47,7 @@ int DoMain() { const PendulumState& xdot = PendulumPlant::get_state(*derivatives); - // NOLINTNEXTLINE(build/namespaces) Usage documented by fmt library. + // NOLINTNEXTLINE(build/namespaces_literals) Usage documented by fmt library. using namespace fmt::literals; // Derivatives values fmt::print("Forward dynamics, ẋ = [θ̇, ω̇]:\n"); diff --git a/tools/lint/cpplint.bzl b/tools/lint/cpplint.bzl index f44df55f1633..25a42c6ff3d0 100644 --- a/tools/lint/cpplint.bzl +++ b/tools/lint/cpplint.bzl @@ -73,10 +73,10 @@ def _add_linter_rules( # Google cpplint. py_test_isolated( name = name + "_cpplint", - srcs = ["@styleguide//:cpplint"], + srcs = ["@cpplint_internal//:cpplint"], data = cpplint_data + source_labels, args = _EXTENSIONS_ARGS + source_filenames, - main = "@styleguide//:cpplint/cpplint.py", + main = "@cpplint_internal//:cpplint.py", size = size, tags = ["cpplint", "lint"], ) diff --git a/tools/workspace/cpplint_internal/BUILD.bazel b/tools/workspace/cpplint_internal/BUILD.bazel new file mode 100644 index 000000000000..67914ea7e0a0 --- /dev/null +++ b/tools/workspace/cpplint_internal/BUILD.bazel @@ -0,0 +1,3 @@ +load("//tools/lint:lint.bzl", "add_lint_tests") + +add_lint_tests() diff --git a/tools/workspace/cpplint_internal/package.BUILD.bazel b/tools/workspace/cpplint_internal/package.BUILD.bazel new file mode 100644 index 000000000000..c68678725bff --- /dev/null +++ b/tools/workspace/cpplint_internal/package.BUILD.bazel @@ -0,0 +1,17 @@ +# -*- bazel -*- + +load("@drake//tools/skylark:py.bzl", "py_binary") + +licenses(["notice"]) # BSD-3-Clause + +package(default_visibility = ["//visibility:public"]) + +py_binary( + name = "cpplint", + srcs = [ + "cpplint.py", + ], + main = "cpplint.py", + python_version = "PY3", + srcs_version = "PY3", +) diff --git a/tools/workspace/cpplint_internal/patches/transitive_includes.patch b/tools/workspace/cpplint_internal/patches/transitive_includes.patch new file mode 100644 index 000000000000..68a3e523dd85 --- /dev/null +++ b/tools/workspace/cpplint_internal/patches/transitive_includes.patch @@ -0,0 +1,76 @@ +diff --git a/cpplint.py b/cpplint.py +index 7a8b8ab..6ce7164 100755 +--- cpplint.py ++++ cpplint.py +@@ -6166,6 +6166,34 @@ def FilesBelongToSameModule(filename_cc, filename_h): + return files_belong_to_same_module, common_path + + ++def UpdateIncludeState(filename, include_dict, io=codecs): ++ """Fill up the include_dict with new includes found from the file. ++ Args: ++ filename: the name of the header to read. ++ include_dict: a dictionary in which the headers are inserted. ++ io: The io factory to use to read the file. Provided for testability. ++ Returns: ++ True if a header was successfully added. False otherwise. ++ """ ++ headerfile = None ++ try: ++ with io.open(filename, 'r', 'utf8', 'replace') as headerfile: ++ linenum = 0 ++ for line in headerfile: ++ linenum += 1 ++ clean_line = CleanseComments(line) ++ match = _RE_PATTERN_INCLUDE.search(clean_line) ++ if match: ++ include = match.group(2) ++ include_dict.setdefault(include.strip('<>"'), linenum) ++ return True ++ except IOError: ++ return False ++ ++ ++print("THIS VERSION OF CPPLINT HAS BEEN MODIFIED BY HAND") ++ ++ + def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, + io=codecs): + """Reports for missing stl includes. +@@ -6221,10 +6249,36 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, + if prefix.endswith('std::') or not prefix.endswith('::'): + required[header] = (linenum, template) + ++ # The policy is that if you #include something in foo.h you don't need to ++ # include it again in foo.cc. Here, we will look at possible includes. + # Let's flatten the include_state include_list and copy it into a dictionary. + include_dict = dict([item for sublist in include_state.include_list + for item in sublist]) + ++ # Did we find the header for this file (if any) and successfully load it? ++ header_found = False ++ ++ # Use the absolute path so that matching works properly. ++ abs_filename = FileInfo(filename).FullName() ++ ++ # For Emacs's flymake. ++ # If cpplint is invoked from Emacs's flymake, a temporary file is generated ++ # by flymake and that file name might end with '_flymake.cc'. In that case, ++ # restore original file name here so that the corresponding header file can be ++ # found. ++ # e.g. If the file name is 'foo_flymake.cc', we should search for 'foo.h' ++ # instead of 'foo_flymake.h' ++ abs_filename = re.sub(r'_flymake\.cc$', '.cc', abs_filename) ++ ++ # include_dict is modified during iteration, so we iterate over a copy of ++ # the keys. ++ header_keys = list(include_dict.keys()) ++ for header in header_keys: ++ (same_module, common_path) = FilesBelongToSameModule(abs_filename, header) ++ fullpath = common_path + header ++ if same_module and UpdateIncludeState(fullpath, include_dict, io): ++ header_found = True ++ + # All the lines have been processed, report the errors found. + for header in sorted(required, key=required.__getitem__): + template = required[header][1] diff --git a/tools/workspace/cpplint_internal/patches/whitespace_newline.patch b/tools/workspace/cpplint_internal/patches/whitespace_newline.patch new file mode 100644 index 000000000000..20be7c3c469d --- /dev/null +++ b/tools/workspace/cpplint_internal/patches/whitespace_newline.patch @@ -0,0 +1,49 @@ +Address discrepancy between cpplint upstream and Google styleguide. + +Google styleguide allows for certain simple if statements to fit on +one line with spaces between the curly braces. While this is technically +a violation of the general styleguide, Google permits this exception +"for historical reasons." Nevertheless, cpplint does not allow this. + +The fix basically adds a "made-up" error category and diverts the +expressions that trigger the aforementioned cases out of the more +general "whitespace/newline" category. The "made-up" name is then +added to the "_DEFAULT_FILTERS" list, effectively suppressing the +error violations in these specific cases. + +diff --git a/cpplint.py b/cpplint.py +index 7a8b8ab..130fd9d 100755 +--- cpplint.py ++++ cpplint.py +@@ -355,6 +355,7 @@ _ERROR_CATEGORIES = [ + 'whitespace/indent_namespace', + 'whitespace/line_length', + 'whitespace/newline', ++ 'whitespace/newline_controlled_statement', + 'whitespace/operators', + 'whitespace/parens', + 'whitespace/semicolon', +@@ -414,6 +415,7 @@ _OTHER_NOLINT_CATEGORY_PREFIXES = [ + _DEFAULT_FILTERS = [ + '-build/include_alpha', + '-readability/fn_size', ++ '-whitespace/newline_controlled_statement', + ] + + # The default list of categories suppressed for C (not C++) files. +@@ -4390,13 +4392,13 @@ def CheckBraces(filename, clean_lines, linenum, error): + if keyword := re.search( + r'\b(else if|if|while|for|switch)' # These have parens + r'\s*\(.*\)\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\};]', line): +- error(filename, linenum, 'whitespace/newline', 5, ++ error(filename, linenum, 'whitespace/newline_controlled_statement', 5, + f'Controlled statements inside brackets of {keyword.group(1)} clause' + ' should be on a separate line') + elif keyword := re.search( + r'\b(else|do|try)' # These don't have parens + r'\s*(?:\[\[(?:un)?likely\]\]\s*)?{\s*[^\s\\}]', line): +- error(filename, linenum, 'whitespace/newline', 5, ++ error(filename, linenum, 'whitespace/newline_controlled_statement', 5, + f'Controlled statements inside brackets of {keyword.group(1)} clause' + ' should be on a separate line') + diff --git a/tools/workspace/cpplint_internal/repository.bzl b/tools/workspace/cpplint_internal/repository.bzl new file mode 100644 index 000000000000..de77affebb37 --- /dev/null +++ b/tools/workspace/cpplint_internal/repository.bzl @@ -0,0 +1,17 @@ +load("//tools/workspace:github.bzl", "github_archive") + +def cpplint_internal_repository( + name, + mirrors = None): + github_archive( + name = name, + repository = "cpplint/cpplint", + commit = "80da3c1ef37af017715dc1366fbda8263179bdeb", + sha256 = "006d8ce87a2dcc8a644b78fe0c2d4d45bc52ca41414ea66d3c7dcd6ae3e61d9e", # noqa + build_file = ":package.BUILD.bazel", + patches = [ + ":patches/transitive_includes.patch", + ":patches/whitespace_newline.patch", + ], + mirrors = mirrors, + ) diff --git a/tools/workspace/default.bzl b/tools/workspace/default.bzl index 9203a6fc5fee..196103e3a184 100644 --- a/tools/workspace/default.bzl +++ b/tools/workspace/default.bzl @@ -16,6 +16,7 @@ load("//tools/workspace/com_jidesoft_jide_oss:repository.bzl", "com_jidesoft_jid load("//tools/workspace/common_robotics_utilities_internal:repository.bzl", "common_robotics_utilities_internal_repository", "common_robotics_utilities_repository") # noqa load("//tools/workspace/commons_io:repository.bzl", "commons_io_repository") load("//tools/workspace/conex_internal:repository.bzl", "conex_internal_repository") # noqa +load("//tools/workspace/cpplint_internal:repository.bzl", "cpplint_internal_repository") # noqa load("//tools/workspace/crate_universe:repository.bzl", "crate_universe_repositories") # noqa load("//tools/workspace/csdp_internal:repository.bzl", "csdp_internal_repository") # noqa load("//tools/workspace/curl_internal:repository.bzl", "curl_internal_repository") # noqa @@ -171,6 +172,8 @@ def add_default_repositories( commons_io_repository(name = "commons_io", mirrors = mirrors, _is_drake_self_call = True) # noqa if "conex_internal" not in excludes: conex_internal_repository(name = "conex_internal", mirrors = mirrors) + if "cpplint_internal" not in excludes: + cpplint_internal_repository(name = "cpplint_internal", mirrors = mirrors) # noqa if "crate_universe" not in excludes: crate_universe_repositories(mirrors = mirrors, excludes = excludes, _is_drake_self_call = True) # noqa if "csdp_internal" not in excludes: diff --git a/tools/workspace/styleguide/BUILD.bazel b/tools/workspace/styleguide/BUILD.bazel index c6fb5c7190d1..67914ea7e0a0 100644 --- a/tools/workspace/styleguide/BUILD.bazel +++ b/tools/workspace/styleguide/BUILD.bazel @@ -1,13 +1,3 @@ load("//tools/lint:lint.bzl", "add_lint_tests") -load("//tools/skylark:py.bzl", "py_test") - -py_test( - name = "cpplint_unittest", - size = "small", - srcs = ["@styleguide//:test_files"], - data = ["@styleguide//:test_files"], - main = "@styleguide//:cpplint/cpplint_unittest.py", - tags = ["no_kcov"], -) add_lint_tests() diff --git a/tools/workspace/styleguide/package.BUILD.bazel b/tools/workspace/styleguide/package.BUILD.bazel index c9ffd8134c9c..2e187ca91837 100644 --- a/tools/workspace/styleguide/package.BUILD.bazel +++ b/tools/workspace/styleguide/package.BUILD.bazel @@ -1,7 +1,5 @@ # -*- bazel -*- -load("@drake//tools/skylark:py.bzl", "py_binary") - licenses(["notice"]) # BSD-3-Clause package(default_visibility = ["//visibility:public"]) @@ -11,30 +9,3 @@ exports_files(glob([ "*", "include/*", ])) - -# We can't set name="cpplint" here because that's the directory name so the -# sandbox gets confused. We'll give it a private name with a public alias. -py_binary( - name = "cpplint_binary", - srcs = ["cpplint/cpplint.py"], - imports = ["cpplint"], - main = "cpplint/cpplint.py", - python_version = "PY3", - srcs_version = "PY3", - visibility = [], -) - -alias( - name = "cpplint", - actual = ":cpplint_binary", -) - -filegroup( - name = "test_files", - testonly = True, - srcs = [ - "cpplint/cpplint.py", - "cpplint/cpplint_test_header.h", - "cpplint/cpplint_unittest.py", - ], -) diff --git a/tools/workspace/styleguide/patches/test_paths.patch b/tools/workspace/styleguide/patches/test_paths.patch deleted file mode 100644 index 541b903c3b67..000000000000 --- a/tools/workspace/styleguide/patches/test_paths.patch +++ /dev/null @@ -1,20 +0,0 @@ -Work around confusing import of cpplint for Python 3.11 - -We have a directory named cpplint and file named cpplint/cpplint.py. - -We want to import the latter, but sometimes we end up with the empty -__init__.py from the former. Force the issue by munging Python's path. - -Reasoning for not upstreaming this patch: Drake-specific build option. - ---- cpplint/cpplint_unittest.py -+++ cpplint/cpplint_unittest.py -@@ -41,7 +41,8 @@ - import sys - import unittest - -+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) - import cpplint - - try: - xrange # Python 2 diff --git a/tools/workspace/styleguide/patches/upstream/sre_deprecation.patch b/tools/workspace/styleguide/patches/upstream/sre_deprecation.patch deleted file mode 100644 index f42f806cbef9..000000000000 --- a/tools/workspace/styleguide/patches/upstream/sre_deprecation.patch +++ /dev/null @@ -1,17 +0,0 @@ -Avoid DeprecationWarning from sre_compile in Python 3.11 - -See https://github.com/cpplint/cpplint/pull/214 for discussion. - -TODO(jwnimmer-tri) Upstream this to our RobotLocomotion/styleguide. - ---- cpplint/cpplint.py -+++ cpplint/cpplint.py -@@ -47,7 +47,7 @@ - import math # for log - import os - import re --import sre_compile -+import re as sre_compile # sre_compile is deprecated - import string - import sys - import unicodedata diff --git a/tools/workspace/styleguide/repository.bzl b/tools/workspace/styleguide/repository.bzl index e11a74415bfb..53f41693016a 100644 --- a/tools/workspace/styleguide/repository.bzl +++ b/tools/workspace/styleguide/repository.bzl @@ -9,9 +9,5 @@ def styleguide_repository( commit = "736a797e4bc25a2c064fee147e659004675b7387", sha256 = "6fb793292a94baf75b9ad1fb91910cf710b5b1a36823e09abf17ffbea8b06aec", # noqa build_file = ":package.BUILD.bazel", - patches = [ - ":patches/upstream/sre_deprecation.patch", - ":patches/test_paths.patch", - ], mirrors = mirrors, )