diff --git a/.clang-format b/.clang-format index c6b65ba8cbd8..6e776d177001 100644 --- a/.clang-format +++ b/.clang-format @@ -1,8 +1,46 @@ +# -*- yaml -*- + +# This file determines clang-format's style settings; for details, refer to +# http://clang.llvm.org/docs/ClangFormatStyleOptions.html + --- BasedOnStyle: Google --- Language: Cpp + # Force pointers to the type for C++. DerivePointerAlignment: false PointerAlignment: Left + +# Specify the #include statement order. This implements the order mandated by +# the Google C++ Style Guide: related header, C headers, C++ headers, library +# headers, and finally the project headers. +# +# To obtain updated lists of system headers used in the below expressions, see: +# http://stackoverflow.com/questions/2027991/list-of-standard-header-files-in-c-and-c/2029106#2029106. +IncludeCategories: + # Spacers used by drake/tools/formatter.py. + - Regex: '^$' + Priority: 15 + - Regex: '^$' + Priority: 25 + - Regex: '^$' + Priority: 35 + - Regex: '^$' + Priority: 45 + # C system headers. + - Regex: '^[<"](aio|arpa/inet|assert|complex|cpio|ctype|curses|dirent|dlfcn|errno|fcntl|fenv|float|fmtmsg|fnmatch|ftw|glob|grp|iconv|inttypes|iso646|langinfo|libgen|limits|locale|math|monetary|mqueue|ndbm|netdb|net/if|netinet/in|netinet/tcp|nl_types|poll|pthread|pwd|regex|sched|search|semaphore|setjmp|signal|spawn|stdalign|stdarg|stdatomic|stdbool|stddef|stdint|stdio|stdlib|stdnoreturn|string|strings|stropts|sys/ipc|syslog|sys/mman|sys/msg|sys/resource|sys/select|sys/sem|sys/shm|sys/socket|sys/stat|sys/statvfs|sys/time|sys/times|sys/types|sys/uio|sys/un|sys/utsname|sys/wait|tar|term|termios|tgmath|threads|time|trace|uchar|ulimit|uncntrl|unistd|utime|utmpx|wchar|wctype|wordexp)\.h[">]$' + Priority: 20 + # C++ system headers (as of C++14). + - Regex: '^[<"](algorithm|array|atomic|bitset|cassert|ccomplex|cctype|cerrno|cfenv|cfloat|chrono|cinttypes|ciso646|climits|clocale|cmath|codecvt|complex|condition_variable|csetjmp|csignal|cstdalign|cstdarg|cstdbool|cstddef|cstdint|cstdio|cstdlib|cstring|ctgmath|ctime|cuchar|cwchar|cwctype|deque|exception|forward_list|fstream|functional|future|initializer_list|iomanip|ios|iosfwd|iostream|istream|iterator|limits|list|locale|map|memory|mutex|new|numeric|ostream|queue|random|ratio|regex|scoped_allocator|set|shared_mutex|sstream|stack|stdexcept|streambuf|string|strstream|system_error|thread|tuple|type_traits|typeindex|typeinfo|unordered_map|unordered_set|utility|valarray|vector)[">]$' + Priority: 30 + # Other libraries' h files (with angles). + - Regex: '^<' + Priority: 40 + # Your project's h files. + - Regex: '^"drake' + Priority: 50 + # Other libraries' h files (with quotes). + - Regex: '^"' + Priority: 41 --- diff --git a/drake/tools/BUILD b/drake/tools/BUILD index 4966b0be11fb..209efa14d71a 100644 --- a/drake/tools/BUILD +++ b/drake/tools/BUILD @@ -17,6 +17,15 @@ py_binary( deps = [ ":named_vector", ], + data = [ + "//tools:clang-format", + ] +) + +py_library( + name = "formatter", + srcs = ["formatter.py"], + data = ["//tools:clang-format"], ) # === test/ === @@ -79,8 +88,13 @@ sh_test( "test/gen/sample_translator.h", "test/sample.named_vector", ":lcm_vector_gen", - "//:.clang-format", ], ) +py_test( + name = "formatter_test", + srcs = ["test/formatter_test.py"], + deps = [":formatter"], +) + cpplint(data = ["test/gen/drake/CPPLINT.cfg"]) diff --git a/drake/tools/formatter.py b/drake/tools/formatter.py new file mode 100755 index 000000000000..cc3d4898e9ec --- /dev/null +++ b/drake/tools/formatter.py @@ -0,0 +1,297 @@ +"""Classes to support C++ code formatting tools. +""" + +import os +from subprocess import Popen, PIPE, CalledProcessError + + +class FormatterBase(object): + """A base class for formatting-related tools, with the ability to load a + file, add / remove / modify lines, clang-format selected regions, and + compare the changes to the original file contents. + + This class models a "working list" of the file during reformatting. Each + line in the file has an index (starting with index 0), and each line ends + with a newline (included, not implicit). Callers can modify the working + list, and run clang-format on (portions of) the working-list. + + The is_same_as_original and is_permutation_of_original methods compare the + current working list to the original file contents. Other methods provide + access and modification by index. + + The should_format predicate can be overridden in subclasses to change which + lines are subject to clang-format modifications. + """ + + def __init__(self, filename, readlines=None): + """Create a new FormatterBase. The required filename parameter refers + to the workspace-relative full path, e.g. drake/common/drake_assert.h. + + The readlines parameter is optional and useful mostly for unit testing. + When readlines is present, readlines becomes the work list and the + filename is not opened. When readlines is absent, this constructor + reads the contents of the filename from disk into the work list (i.e., + the default value of readlines is open(filename).readlines()). + """ + self._filename = filename + if readlines is None: + with open(filename, "r") as opened: + self._original_lines = opened.readlines() + else: + self._original_lines = list(readlines) + self._working_lines = list(self._original_lines) + self._check_rep() + + def _check_rep(self): + assert self._filename + for line in self._original_lines: + assert line.endswith("\n"), line + for line in self._working_lines: + assert line.endswith("\n"), line + + def is_same_as_original(self): + """Return whether the working list is identical to the original file + contents read in by (or passed to) the constructor. + """ + return self._working_lines == self._original_lines + + def is_permutation_of_original(self): + """Return whether the working list is a permultation of the original + file lines read in by (or passed to) the constructor, modulo blank + lines. + """ + return ( + set(self._working_lines + ["\n"]) == + set(self._original_lines + ["\n"])) + + def is_valid_index(self, index): + return 0 <= index and index < len(self._working_lines) + + def is_blank_line(self, index): + return self.get_line(index).strip() == "" + + def get_num_lines(self): + return len(self._working_lines) + + def get_line(self, index): + assert self.is_valid_index(index) + return self._working_lines[index] + + def get_all_lines(self): + return list(self._working_lines) + + def set_line(self, index, line): + assert self.is_valid_index(index) + self._working_lines[index] = line + self._check_rep() + + def set_all_lines(self, lines): + self._working_lines = list(lines) + self._check_rep() + + def insert_lines(self, index, lines): + assert 0 <= index and index <= len(self._working_lines) + self._working_lines[index:0] = lines + self._check_rep() + + def remove_all(self, indices): + if len(indices) == 0: + return + assert len(set(indices)) == len(indices) + rev_sorted_indices = sorted(indices, reverse=True) + assert self.is_valid_index(rev_sorted_indices[0]) + assert self.is_valid_index(rev_sorted_indices[-1]) + for index in rev_sorted_indices: + del self._working_lines[index] + + def should_format(self, clang_format_on, index, line): + """Subclasses can override to change what's going to be formatted. + True means the line should be formatted. The default is the same as + clang-format -- everything goes except "clang-format off" sections. + """ + return clang_format_on + + def get_format_indices(self): + """Return the sorted list of all indices that pass the + self.should_format predicate. + """ + result = [] + clang_format_on = True + for index, line in enumerate(self._working_lines): + # Ignore lines between clang-format directive comments, and also + # ignore the lines with the directive comments themselves. + if '// clang-format off' in line: + clang_format_on = False + continue + if '/* clang-format off */' in line: + clang_format_on = False + continue + elif '// clang-format on' in line: + clang_format_on = True + continue + elif '/* clang-format on */' in line: + clang_format_on = True + continue + if self.should_format(clang_format_on, index, line): + result.append(index) + return result + + def get_non_format_indices(self): + """Return the complement of get_format_indices(). + """ + all_indices = set(xrange(len(self._working_lines))) + return sorted(all_indices - set(self.get_format_indices())) + + @staticmethod + def indices_to_ranges(indices): + """Group a list of sorted indices into a sorted list of softed lists of + adjacent indices. + + For example [1, 2, 5, 7, 8, 9] yields [[1, 2], [5], [7, 8, 9]]. + """ + assert indices == sorted(indices) + result = [] + for i in indices: + if len(result) and (result[-1][-1] == (i - 1)): + # Grow an existing range. + result[-1] = result[-1] + [i] + else: + # Start a new range. + result.append([i]) + return result + + def get_format_ranges(self): + return self.indices_to_ranges(self.get_format_indices()) + + def get_non_format_ranges(self): + return self.indices_to_ranges(self.get_non_format_indices()) + + def clang_format(self): + """Reformat the working list using clang-format, passing -lines=... + groups for only the lines that pass the should_format() predicate. + """ + # Convert format_ranges to clang's one-based indexing. + lines_args = ["-lines=%d:%d" % (one_range[0] + 1, one_range[-1] + 1) + for one_range in self.get_format_ranges()] + if not lines_args: + return + + # Run clang-format. + command = [ + "clang-format", + "-style=file", + "-assume-filename=%s" % self._filename] + \ + lines_args + formatter = Popen(command, stdin=PIPE, stdout=PIPE) + stdout, _ = formatter.communicate(input="".join(self._working_lines)) + + # Handle errors, otherwise reset the working list. + if formatter.returncode != 0: + raise CalledProcessError(formatter.returncode, command, stdout) + self.set_all_lines(stdout.splitlines(True)) + + def _pre_rewrite_file(self): + """Subclasses may override to perform actions prior to rewrite_file.""" + pass + + def rewrite_file(self): + """Overwrite the contents of the filename passed into the constructor + with the current working list. + """ + self._pre_rewrite_file() + temp_filename = self._filename + ".drake-formatter" + with open(temp_filename, "w") as opened: + for line in self._working_lines: + opened.write(line) + os.rename(temp_filename, self._filename) + + +class IncludeFormatter(FormatterBase): + """A formatter tool that provides format_includes() helper to sort the + #include statements to match Google C++ Style Guide, as well as put a + blank line between each of the #include group sections. + + """ + + def __init__(self, filename, *args, **kwargs): + super(IncludeFormatter, self).__init__(filename, *args, **kwargs) + self._related_headers = [] + + # In most cases, clang-format has a built-in IncludeIsMainRegex that + # identifies the "related header" correctly, automatically. For an + # inl-h file, clang will not realize the .h is "related", so instead we + # manually compute that the related-header pathname here. + if filename.endswith('-inl.h'): + prior_to_ext = filename[:-len('-inl.h')] + self._related_headers.append('#include "%s.h"\n' % prior_to_ext) + + def _pre_rewrite_file(self): + # Sanity check before writing out again. + if self.is_permutation_of_original(): + return + message = "" + message += "%s: changes were not just a shuffle\n" % self._filename + message += "=" * 78 + "\n" + message += "".join(self._original_lines) + message += "=" * 78 + "\n" + message += "".join(self._working_lines) + message += "=" * 78 + "\n" + raise Exception(message) + + def should_format(self, clang_format_on, index, line): + # We want all include statements, but until clang-format gets + # IncludeIsMainRegex support, we need omit the "related header"s. + return ( + clang_format_on and + line.startswith("#include") and + '-inl.h"' not in line and + line not in self._related_headers) + + def format_includes(self): + """Reformat the #include statements in the working list (possibly also + changing around some nearby blank lines). + """ + # If there are not any includes to format, then we are done. + if not self.get_format_indices(): + return + + # Remove blank lines that are fully surrounded by include statements + # (or bracketed by include statements and start- or end-of-file). We + # will put back blank lines later, but in the meantime we need all of + # the includes bunched together so that clang-format will sort them. + # We want to preserve the comment line layouts near include statements, + # so here only _completely_ empty ranges are removed. + blank_indices_to_remove = [] + for one_range in self.get_non_format_ranges(): + if all([self.is_blank_line(index) for index in one_range]): + blank_indices_to_remove.extend(one_range) + self.remove_all(blank_indices_to_remove) + + # Add priority spacers after every group of #include statements. We + # will turn these spacers into whitespace separators when we're done. + SPACERS = ["#include \n" % priority + for priority in [15, 25, 35, 45]] + for one_range in reversed(self.get_format_ranges()): + last_include_index = one_range[-1] + self.insert_lines(last_include_index + 1, SPACERS) + + # Run the formatter over only the in-scope #include statements. + self.clang_format() + + # Turn each run of spacers within an include block into a blank line. + # Remove any runs of spaces at the start or end. + include_indices = self.get_format_indices() + spacer_indices_to_remove = [] + spacer_ranges = self.indices_to_ranges([ + i for i in include_indices + if self.get_line(i) in SPACERS]) + for one_range in spacer_ranges: + before_spacer = one_range[0] - 1 + after_spacer = one_range[-1] + 1 + if all([x in include_indices + for x in [before_spacer, after_spacer]]): + # Interior group of spacers. Replace with a blank line. + self.set_line(one_range[0], "\n") + one_range = one_range[1:] + spacer_indices_to_remove.extend(one_range) + self.remove_all(spacer_indices_to_remove) diff --git a/drake/tools/test/formatter_test.py b/drake/tools/test/formatter_test.py new file mode 100755 index 000000000000..aa1ea92c44a4 --- /dev/null +++ b/drake/tools/test/formatter_test.py @@ -0,0 +1,288 @@ +import unittest + +from drake.tools.formatter import FormatterBase, IncludeFormatter + + +class TestFormatterBase(unittest.TestCase): + + def test_essentials(self): + original_lines = [ + '// Line 1\n', + '/* Line 2 */\n', + '\n', + ] + dut = FormatterBase('filename.cc', readlines=original_lines) + + # Everything starts out unchanged. + self.assertTrue(dut.is_same_as_original()) + self.assertTrue(dut.is_permutation_of_original()) + self.assertEqual(dut.get_all_lines(), original_lines) + + # Basic getters. + self.assertEqual(dut.get_num_lines(), 3) + self.assertTrue(dut.is_blank_line(2)) + self.assertEqual(dut.get_line(0), '// Line 1\n') + + # Reverse it and end up with a permutation. + dut.set_all_lines(reversed(dut.get_all_lines())) + self.assertFalse(dut.is_same_as_original()) + self.assertTrue(dut.is_permutation_of_original()) + + # Rebuild it using insertion and removal. + dut.set_all_lines(['\n'] * 3) + dut.set_line(0, '/* Line 2 */\n') + dut.insert_lines(0, ['AAA\n', '// Line 1\n']) + dut.remove_all([0, 3]) + self.assertEqual(dut.get_all_lines(), original_lines) + + def test_format_ranges(self): + original_lines = [ + '#include "line0"\n', + '// clang-format off\n', + '#include "line2"\n', + '// clang-format on\n', + '#include "line4"\n', + '#include "line5"\n', + '/* clang-format off */\n', + '#include "line7"\n', + '#include "line8"\n', + '/* clang-format on */\n', + '#include "line10"\n', + ] + dut = FormatterBase("filename.cc", readlines=original_lines) + + self.assertEqual( + dut.get_format_ranges(), [[0], [4, 5], [10]]) + self.assertEqual( + dut.get_non_format_ranges(), [[1, 2, 3], [6, 7, 8, 9]]) + + +class TestIncludeFormatter(unittest.TestCase): + + def _split(self, triple_quoted_file_contents): + lines = triple_quoted_file_contents.split("\n") + assert len(lines) >= 2 + assert lines[0] == "" # Detritus from first triple quote. + assert lines[-1] == "" # Detritus from last triple quote. + del lines[0] + del lines[-1] + return [line + "\n" for line in lines] + + def _check(self, basename, original, expected): + original_lines = self._split(original) + expected_lines = self._split(expected) + dut = IncludeFormatter( + "drake/dummy/" + basename, + readlines=original_lines) + dut.format_includes() + self.assertEquals(dut.get_all_lines(), expected_lines) + + def test_basic(self): + # A pile of headers gets sorted per cppguide: + # - The related header + # - C system files + # - C++ system files + # - Other libraries' .h files + # - Your project's .h files + original = """ +#include "drake/common/drake_assert.h" +#include "drake/dummy/bar.h" +#include "drake/dummy/dut.h" +#include +#include +#include +#include +#include +#include +""" + expected = """ +#include "drake/dummy/dut.h" + +#include +#include + +#include +#include + +#include +#include + +#include "drake/common/drake_assert.h" +#include "drake/dummy/bar.h" +""" + self._check("dut.cc", original, expected) + + def test_nothing(self): + # A file with _no_ include statements. + original = """ +namespace { } +""" + self._check("dut.cc", original, original) + + def test_regroup(self): + # Wrongly grouped whitespace. + original = """ +#include "drake/dummy/dut.h" + +#include +#include +#include + +#include "drake/common/drake_assert.h" +#include "drake/dummy/bar.h" +#include +""" + expected = """ +#include "drake/dummy/dut.h" + +#include +#include + +#include +#include + +#include "drake/common/drake_assert.h" +#include "drake/dummy/bar.h" +""" + self._check("dut.cc", original, expected) + + def test_format_off(self): + # "clang-format off". + original = """ +#include "drake/dummy/dut.h" + +// clang-format off +#ifdef FOO +#include +#include +#else +#include +#include +#endif +// clang-format on + +#include "drake/common/drake_assert.h" +""" + self._check("dut.cc", original, original) + + def test_cc_uses_single_inl(self): + # Only a single "-inl.h" include. + original = """ +#include "drake/dummy/dut-inl.h" + +namespace { } +""" + self._check("dut.cc", original, original) + + def test_cc_uses_inl_and_more(self): + # Presence of "-inl.h" pattern and other things. + original = """ +#include "drake/dummy/dut-inl.h" + +#include "drake/common/drake_assert.h" +#include "drake/common/drake_deprecated.h" + +namespace { } +""" + self._check("xxx.cc", original, original) + + def test_target_is_header(self): + # A header file. + original = """ +#include "drake/common/drake_assert.h" +#include + +namespace { } +""" + expected = """ +#include + +#include "drake/common/drake_assert.h" + +namespace { } +""" + self._check("dut.h", original, expected) + + def test_target_is_inl(self): + # An *-inl.h file. + original = """ +#include "drake/dummy/dut.h" + +#include + +namespace { } +""" + self._check("dut-inl.h", original, original) + + def test_associated_comment(self): + # A comment prior to a line. + original = """ +#include "drake/dummy/dut.h" + +// Some comment describing the next line. +#include + +namespace { } +""" + self._check("dut.cc", original, original) + + def test_file_opening_comment(self): + # A comment atop the file with no blank line. + original = """ +/// @file dut.cc +/// Mumble mumble +/// +#include +#include +""" + self._check("dut.cc", original, original) + + def test_internal_related_header(self): + # Two related headers, guarded by "clang-format off". + original = """ +/* clang-format off */ +#include "drake/dummy/dut.h" +#include "drake/dummy/dut_internal.h" +/* clang-format on */ + +#include +#include + +#include "drake/dummy/drake_assert.h" +#include "drake/dummy/drake_deprecated.h" +""" + self._check("dut.cc", original, original) + + def test_resort_solo_groups(self): + # Groups of one, but sorted uncorrectly. + original = """ +#include "drake/dummy/dut.h" + +#include "drake/common/drake_assert.h" + +#include +""" + expected = """ +#include "drake/dummy/dut.h" + +#include + +#include "drake/common/drake_assert.h" +""" + self._check("dut.cc", original, expected) + + def test_nontrivial_reformatting(self): + # If clang-format changes any lines, we want to fail-fast. + # (Note the two spaces between #include and the double quote.) + original_lines = ['#include "nontrivial.h"\n'] + dut = IncludeFormatter("nontrivial.cc", readlines=original_lines) + dut.format_includes() + with self.assertRaisesRegexp(Exception, 'not just a shuffle'): + dut.rewrite_file() + + +# TODO(jwnimmer-tri) Omitting or mistyping these lines means that no tests get +# run, and nobody notices. We should probably have drake_py_unittest macro +# that takes care of this, to be less brittle. +if __name__ == '__main__': + unittest.main() diff --git a/drake/tools/test/lcm_vector_gen_test.sh b/drake/tools/test/lcm_vector_gen_test.sh index 91b0e5600538..f8560139a6c5 100755 --- a/drake/tools/test/lcm_vector_gen_test.sh +++ b/drake/tools/test/lcm_vector_gen_test.sh @@ -3,34 +3,37 @@ set -ex -mkdir tmp - # Dump some debugging output. find . +# Move the originals (which are symlinks) out of the way. +tool_outputs="lcmt_sample_t.lcm sample.cc sample.h sample_translator.cc sample_translator.h" +for item in $tool_outputs; do + mv drake/tools/test/gen/"$item"{,.orig} +done + # Run the code generator. # TODO(jwnimmer-tri) De-duplicate this with lcm_vector_gen.sh. ./drake/tools/lcm_vector_gen \ - --lcmtype-dir="tmp" \ - --cxx-dir="tmp" \ + --lcmtype-dir="drake/tools/test/gen" \ + --cxx-dir="drake/tools/test/gen" \ --namespace="drake::tools::test" \ --title="Sample" \ --workspace=$(pwd) \ --named_vector_file="drake/tools/test/sample.named_vector" # Dump some debugging output. -find tmp +find drake/tools/test/gen # Re-format the code. # TODO(jwnimmer-tri) De-duplicate this with lcm_vector_gen.sh. -clang-format --style=file -i tmp/*.h tmp/*.cc +for item in $tool_outputs; do + clang-format --style=file -i drake/tools/test/gen/"$item" +done # Insist that the generated output matches the current copy in git. -for item in lcmt_sample_t.lcm sample.cc sample.h sample_translator.cc sample_translator.h; do - # Fuzz out a desirable difference in the generated code. - sed -i -e 's|include "tmp|include "drake/tools/test/gen|' tmp/"$item" - # The files should be identical now. - diff --unified=20 drake/tools/test/gen/"$item" tmp/"$item" +for item in ; do + diff --unified=20 drake/tools/test/gen/"$item"{.orig,} done echo "PASS" diff --git a/tools/BUILD b/tools/BUILD index c81da1a74ef0..3d2f645332f7 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -2,6 +2,13 @@ package(default_visibility = ["//visibility:public"]) +# Depend on this when you need to call clang-format. (We don't have a label +# for the binary yet, but this will stand in for it in the meantime.) +filegroup( + name = "clang-format", + srcs = ["//:.clang-format"], +) + sh_binary( name = "valgrind", srcs = ["valgrind.sh"],