Skip to content

Commit

Permalink
Prevent deleting duplicate nested forward decls
Browse files Browse the repository at this point in the history
fix_includes.py could be over-zealous and delete forward declarations of
nested classes if they happened to share a name with another nested
class in the same file.  This was because the analysis of top-level
spans detected #ifdefs and namespaces, but not classes as things which
made code non-top-level.

Luckily, the IWYU output makes it obvious when a forward declaration is
for a nested class.  Take advantage of that to fix this bug.

Also, add a regression test.
  • Loading branch information
jbytheway authored and kimgr committed Mar 22, 2020
1 parent 1d22c11 commit f3bec0d
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 4 deletions.
28 changes: 24 additions & 4 deletions fix_includes.py
Expand Up @@ -235,6 +235,10 @@ def __init__(self, filename):
# report, though, forward-declares inside '#if 0' or similar.)
self.seen_forward_declare_lines = set()

# Those spans which pertain to nested forward declarations (i.e. of nested
# classes). This set should be a subset of self.seen_forward_declare_lines.
self.nested_forward_declare_lines = set()

# A set of each line in the iwyu 'add' section.
self.includes_and_forward_declares_to_add = OrderedSet()

Expand All @@ -261,6 +265,7 @@ def Merge(self, other):
self.lines_to_delete.intersection_update(other.lines_to_delete)
self.some_include_lines.update(other.some_include_lines)
self.seen_forward_declare_lines.update(other.seen_forward_declare_lines)
self.nested_forward_declare_lines.update(other.nested_forward_declare_lines)
self.includes_and_forward_declares_to_add.update(
other.includes_and_forward_declares_to_add)
self.full_include_lines.update(other.full_include_lines)
Expand Down Expand Up @@ -443,8 +448,10 @@ def ParseOneRecord(self, iwyu_output, flags):
continue
m = self._LINE_NUMBERS_COMMENT_RE.search(line)
if m:
retval.seen_forward_declare_lines.add((int(m.group(1)),
int(m.group(2))+1))
line_range = (int(m.group(1)), int(m.group(2))+1)
retval.seen_forward_declare_lines.add(line_range)
if '::' in line:
retval.nested_forward_declare_lines.add(line_range)

# IWYUOutputRecord.includes_and_forward_declares_to_add
for line in self.lines_by_section.get(self._ADD_SECTION_RE, []):
Expand Down Expand Up @@ -503,6 +510,10 @@ def __init__(self, line):
# of the class/struct. For other types of lines, this is None.
self.key = None

# If this is a forward-declaration of a nested class, then this will be
# True.
self.is_nested_forward_declaration = False

def __str__(self):
if self.deleted:
line = 'XX-%s-XX' % self.line
Expand Down Expand Up @@ -771,6 +782,13 @@ def _CalculateLineTypesAndKeys(file_lines, iwyu_record):
% (iwyu_record.filename, line_number))
file_lines[line_number].type = _FORWARD_DECLARE_RE

for (start_line, end_line) in iwyu_record.nested_forward_declare_lines:
for line_number in range(start_line, end_line):
if line_number >= len(file_lines):
raise FixIncludesError('iwyu line number %s:%d is past file-end'
% (iwyu_record.filename, line_number))
file_lines[line_number].is_nested_forward_declaration = True

# While we're at it, let's do a bit more sanity checking on iwyu_record.
for line_number in iwyu_record.lines_to_delete:
if line_number >= len(file_lines):
Expand Down Expand Up @@ -1281,7 +1299,8 @@ def _ShouldInsertBlankLine(decorated_move_span, next_decorated_move_span,


def _GetToplevelReorderSpans(file_lines):
"""Returns a sorted list of all reorder_spans not inside an #ifdef/namespace.
"""Returns a sorted list of all reorder_spans not inside an
#ifdef/namespace/class.
This routine looks at all the reorder_spans in file_lines, ignores
reorder spans inside #ifdefs and namespaces -- except for the 'header
Expand Down Expand Up @@ -1337,7 +1356,8 @@ def _GetToplevelReorderSpans(file_lines):
good_reorder_spans = []
for reorder_span in reorder_spans:
for line_number in range(*reorder_span):
if in_ifdef[line_number] or in_namespace[line_number]:
if (in_ifdef[line_number] or in_namespace[line_number] or
file_lines[line_number].is_nested_forward_declaration):
break
else: # for/else
good_reorder_spans.append(reorder_span) # never in ifdef or namespace
Expand Down
28 changes: 28 additions & 0 deletions fix_includes_test.py
Expand Up @@ -2945,6 +2945,34 @@ class B;
self.RegisterFileContents({'dont_remove_template_lines': infile})
self.ProcessAndTest(iwyu_output)

def testDontRemoveSimilarNestedDeclarations(self):
"""Tests we don't accidentally think repeated nested forward declarations
are dupes."""
infile = """\
#include <notused.h> ///-
class A {
class Inner;
};
class B {
class Inner;
};
"""
iwyu_output = """\
dont_remove_similar_nested should add these lines:
dont_remove_similar_nested should remove these lines:
- #include <notused.h> // lines 1-1
The full include-list for dont_remove_similar_nested:
class A::Inner; // lines 4-4
class B::Inner; // lines 8-8
---
"""
self.RegisterFileContents({'dont_remove_similar_nested': infile})
self.ProcessAndTest(iwyu_output)

def testNestedNamespaces(self):
infile = """\
// Copyright 2010
Expand Down

0 comments on commit f3bec0d

Please sign in to comment.