From e3fbe30e0414b7437d8e1b9f636239b2309db3d7 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Tue, 7 Jun 2022 06:42:53 -0700 Subject: [PATCH] [workspace] Make vendor_cxx smarter Reduce the flapping of open/close namespaces. No need to wrap blank lines nor preprocessor directives. --- tools/workspace/vendor_cxx.py | 168 +++++++++++++++++++++++++---- tools/workspace/vendor_cxx_test.py | 87 +++++++++++++-- 2 files changed, 221 insertions(+), 34 deletions(-) diff --git a/tools/workspace/vendor_cxx.py b/tools/workspace/vendor_cxx.py index 1e8c590f3bc6..4a148721f995 100644 --- a/tools/workspace/vendor_cxx.py +++ b/tools/workspace/vendor_cxx.py @@ -8,17 +8,138 @@ """ import argparse +from enum import Enum +import re + + +def _designate_wrapped_lines(lines): + """Given a list[str] of the lines in a C++ source file, returns a + list[bool] that is True iff the corresponding line should be part + of the inline hidden namespace. + + We MUST wrap all C/C++ code. We must NOT wrap #include statements. + + Blank lines and other non-C++ lines such as comments or non-#include + preprocessor directives can go either way; we'll group them into the + wrapping status of their neighbors in order to minimize the number of + added lines. + """ + class Flag(Enum): + WRAP = 1 + NO_WRAP = -1 + DONT_CARE = 0 + + # Regexs to match various kinds of code patterns. + is_include = re.compile(r'^\s*#\s*include\s*["<].*$') + is_preprocessor = re.compile(r'^\s*#\s*[a-zA-Z].*$') + is_blank = re.compile(r'^\s*$') + is_blank_cpp_comment = re.compile(r'^\s*//.*$') + is_blank_c_comment_begin = re.compile(r'^\s*/\*.*$') + is_c_comment_end = re.compile(r'^.*\*/\s*(.*)$') + + # Loop over all lines and determine each one's flag. + flags = [None] * len(lines) + i = 0 + while i < len(lines): + line = lines[i] + # When the prior line has continuation, this line inherits its Flag. + if i > 0 and lines[i - 1].endswith('\\'): + flags[i] = flags[i - 1] + i += 1 + continue + # We must NOT wrap #include statements. + if is_include.match(line): + flags[i] = Flag.NO_WRAP + i += 1 + continue + # Other preprocessor directives can go either way. + if is_preprocessor.match(line): + flags[i] = Flag.DONT_CARE + i += 1 + continue + # Blank lines (or lines that are blank other than their comments) + # can go either way. + if is_blank.match(line) or is_blank_cpp_comment.match(line): + flags[i] = Flag.DONT_CARE + i += 1 + continue + # For C-style comments, consume the entire comment block immediately. + if is_blank_c_comment_begin.match(line): + first_c_comment_line = i + while True: + line = lines[i] + match = is_c_comment_end.match(line) + flags[i] = Flag.DONT_CARE + i += 1 + if match: + break + # If the close-comment marker had code after it, we need to go back + # and set the entire C-style comment to WRAP. + (trailing,) = match.groups() + if trailing: + for fixup in range(first_c_comment_line, i): + flags[fixup] = Flag.WRAP + continue + # We MUST wrap all C/C++ code. + flags[i] = Flag.WRAP + i += 1 + + # We want to insert inline namespaces such that: + # + # - all WRAP lines are enclosed; + # - no NO_WRAP lines are enclosed; + # - the only DONT_CARE lines enclosed are surrouneded by WRAP. + # + # We'll do that by growing the NO_WRAP spans as large as possible. + + # Grow the start-of-file run of NO_WRAP: + for i in range(len(flags)): + if flags[i] == Flag.DONT_CARE: + flags[i] = Flag.NO_WRAP + else: + break + + # Grow the end-of-file run of NO_WRAP: + for i in range(len(flags) - 1, -1, -1): + if flags[i] == Flag.DONT_CARE: + flags[i] = Flag.NO_WRAP + else: + break + + # Grow any interior regions of NO_WRAP: + for i in range(len(flags)): + if flags[i] == Flag.NO_WRAP: + # Change all of the immediately prior and subsequent homogeneous + # runs of DONT_CARE to NO_WRAP. + for j in range(i - 1, -1, -1): + if flags[j] == Flag.DONT_CARE: + flags[j] = Flag.NO_WRAP + else: + break + for j in range(i + 1, len(flags)): + if flags[j] == Flag.DONT_CARE: + flags[j] = Flag.NO_WRAP + else: + break + + # Anything remaining is DONT_CARE bookended by WRAP, so we'll WRAP it. + for i in range(len(flags)): + if flags[i] == Flag.DONT_CARE: + flags[i] = Flag.WRAP + + # Return True only for the wrapped lines. + return [x == Flag.WRAP for x in flags] def _rewrite_one_text(*, text, edit_include): """Rewrites the C++ file contents in `text` with specific alterations: - The paths in #include statements are replaced per the (old, new) pairs in - the include_edit list. Only includes that use quotation marks will be - changed. The pairs are literal strings that must be a prefix of the path. + the include_edit list. The pairs are literal strings that must be a prefix + of the path. - Wraps an inline namespace "drake_vendor" with hidden symbol visibility - around the entire file; it is withdrawn prior to any include statement. + around all of the code in file (but not any #include statements). Returns the new C++ contents. @@ -29,30 +150,31 @@ def _rewrite_one_text(*, text, edit_include): # Re-spell the project's own include statements. for old_inc, new_inc in edit_include: text = text.replace(f'#include "{old_inc}', f'#include "{new_inc}') + text = text.replace(f'#include <{old_inc}', f'#include <{new_inc}') - # Add an inline namespace around the whole file, but disable it around - # include statements. + # We'll add an inline namespace around the C++ code in this file. + # Designate each line of the file for whether it should be wrapped. + lines = text.split('\n') + if lines[-1] == '': + lines.pop() + should_wrap = _designate_wrapped_lines(lines) + + # Anytime the sense of wrapping switches, we'll insert a line. + # Do this in reverse order so that the indices into lines[] are stable. open_inline = ' '.join([ 'inline namespace drake_vendor', '__attribute__ ((visibility ("hidden")))', - '{\n']) - close_inline = '} /* inline namespace drake_vendor */\n' - text = open_inline + text + close_inline - search_start = 0 - while True: - i = text.find('\n#include ', search_start) - if i < 0: - break - first = i + 1 - last = text.index('\n', first) + 1 - text = ( - text[:first] - + close_inline - + text[first:last] - + open_inline - + text[last:]) - search_start = last + len(open_inline) + len(close_inline) - 1 - + '{']) + close_inline = '} /* inline namespace drake_vendor */' + for i in range(len(lines), -1, -1): + this_wrap = should_wrap[i] if i < len(lines) else False + prior_wrap = should_wrap[i - 1] if i > 1 else False + if this_wrap == prior_wrap: + continue + insertion = open_inline if this_wrap else close_inline + lines.insert(i, insertion) + + text = '\n'.join(lines) + '\n' return text diff --git a/tools/workspace/vendor_cxx_test.py b/tools/workspace/vendor_cxx_test.py index a534f1370aa5..d4a9fa7b3a26 100644 --- a/tools/workspace/vendor_cxx_test.py +++ b/tools/workspace/vendor_cxx_test.py @@ -26,23 +26,88 @@ def _check(self, old_lines, expected_new_lines): expected_new_text = '\n'.join(expected_new_lines) + '\n' self.assertMultiLineEqual(expected_new_text, new_text) - def test_simple(self): + def test_comments(self): self._check([ - '#include "somelib/somefile.h"', - '#include "unrelated/thing.h"', + ' // file comment', + ' /* block comment', + ' continues */', + 'int foo(); // eol comment', + 'int bar(); /* eol comment */', + ' /* intro comment */ class Baz;', + ' /* single line c style */', + '#include ', + ' /* intro comment that spans multiple lines', + ' but continues with code after */ class Quux;', + ], [ + ' // file comment', + ' /* block comment', + ' continues */', + self._open, + 'int foo(); // eol comment', + 'int bar(); /* eol comment */', + ' /* intro comment */ class Baz;', + self._close, + ' /* single line c style */', + '#include ', + self._open, + ' /* intro comment that spans multiple lines', + ' but continues with code after */ class Quux;', + self._close, + ]) + + def test_preprocessor(self): + self._check([ + '#define FOO \\', + ' foo', # This line looks like code, but it isn't. 'int foo();', ], [ - # Adjacent pairs of open/close are useless; ideally, we teach - # vendor_cxx to skip these. - self._open, self._close, # TODO(jwnimmer-tri) Useless filler. + '#define FOO \\', + ' foo', + self._open, + 'int foo();', + self._close, + ]) - # All include statements must NOT be within an open-namespace. - '#include "drake_vendor/somelib/somefile.h"', + def test_include_guard(self): + self._check([ + '#ifndef FOO_HH', + '#define FOO_HH', + '#include ', + 'int foo();', + '#endif FOO_HH', + ], [ + '#ifndef FOO_HH', + '#define FOO_HH', + '#include ', + self._open, + 'int foo();', + self._close, + '#endif FOO_HH', + ]) - self._open, self._close, # TODO(jwnimmer-tri) Useless filler. - '#include "unrelated/thing.h"', + def test_pragma_once(self): + self._check([ + '#pragma once', + '#include ', + 'int foo();', + ], [ + '#pragma once', + '#include ', + self._open, + 'int foo();', + self._close, + ]) - # All code MUST be within an open-namespace. + def test_edit_include(self): + self._check([ + '#include "somelib/somefile.h"', + '#include ', + '#include ', + 'int foo();', + ], [ + '#include "drake_vendor/somelib/somefile.h"', + '#include ', + '#include ', self._open, 'int foo();', self._close,