Ensure that |SomeTemplate<SomeClass>* ptr_field| gets rewritten.
It turns out that before this CL |SomeTemplate<SomeClass>* ptr_field|
would match the |non_free_standing_tag_type| matcher and therefore would
not get rewritten. :-(
Both the old |non_free_standing_tag_type| matcher, and the old
|hasUniqueTypeLoc| matcher were trying to prevent generating conflicting
replacements (i.e. separate edits of overlapping sections of a source
file). This CL fixes this problem in a more direct way - by introducing
|overlapsOtherDeclsWithinRecordDecl| matcher for FieldDecls. The new
matcher replaces both of the old matchers.
The new matcher is a more generic version of the old, removed
|hasUniqueTypeLoc| matcher. The new matcher:
1) considers all Decls under a RecordDecl (not just FieldDecls)
2) looks for any SourceRange overlap (rather than just considering
the starting SourceLocation of a field type)
Bug: 1069567
Change-Id: I6392aa84e9e808790be3d84d0ba3079d1cc4ddf8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211054
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777544}
diff --git a/tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp b/tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp
index fa37b45..93b15dc 100644
--- a/tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp
+++ b/tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp
@@ -22,6 +22,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Basic/CharInfo.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/FrontendActions.h"
@@ -216,11 +217,6 @@
clang::Language current_language_ = clang::Language::Unknown;
};
-AST_MATCHER(clang::TagDecl, isNotFreeStandingTagDecl) {
- const clang::TagDecl* tag_decl = Node.getCanonicalDecl();
- return !tag_decl->isFreeStanding();
-}
-
llvm::StringRef GetFilePath(const clang::SourceManager& source_manager,
const clang::FieldDecl& field_decl) {
clang::SourceLocation loc = field_decl.getSourceRange().getBegin();
@@ -438,34 +434,86 @@
return InnerMatcher.matches(*explicit_field_decl, Finder, Builder);
}
-// Matcher for FieldDecl that has a TypeLoc with a unique start location
-// (i.e. has a TypeLoc that is not shared with any other FieldDecl).
+// Returns |true| if and only if:
+// 1. |a| and |b| are in the same file (e.g. |false| is returned if any location
+// is within macro scratch space or a similar location; similarly |false| is
+// returned if |a| and |b| are in different files).
+// 2. |a| and |b| overlap.
+bool IsOverlapping(const clang::SourceManager& source_manager,
+ const clang::SourceRange& a,
+ const clang::SourceRange& b) {
+ clang::FullSourceLoc a1(a.getBegin(), source_manager);
+ clang::FullSourceLoc a2(a.getEnd(), source_manager);
+ clang::FullSourceLoc b1(b.getBegin(), source_manager);
+ clang::FullSourceLoc b2(b.getEnd(), source_manager);
+
+ // Are all locations in a file?
+ if (!a1.isFileID() || !a2.isFileID() || !b1.isFileID() || !b2.isFileID())
+ return false;
+
+ // Are all locations in the same file?
+ if (a1.getFileID() != a2.getFileID() || a2.getFileID() != b1.getFileID() ||
+ b1.getFileID() != b2.getFileID()) {
+ return false;
+ }
+
+ // Check the 2 cases below:
+ // 1. A: |============|
+ // B: |===============|
+ // a1 b1 a2 b2
+ // or
+ // 2. A: |====================|
+ // B: |=======|
+ // a1 b1 b2 a2
+ bool b1_is_inside_a_range = a1.getFileOffset() <= b1.getFileOffset() &&
+ b1.getFileOffset() <= a2.getFileOffset();
+
+ // Check the 2 cases below:
+ // 1. B: |============|
+ // A: |===============|
+ // b1 a1 b2 a2
+ // or
+ // 2. B: |====================|
+ // A: |=======|
+ // b1 a1 a2 b2
+ bool a1_is_inside_b_range = b1.getFileOffset() <= a1.getFileOffset() &&
+ a1.getFileOffset() <= b2.getFileOffset();
+
+ return b1_is_inside_a_range || a1_is_inside_b_range;
+}
+
+// Matcher for FieldDecl that has a SourceRange that overlaps other declarations
+// within the parent RecordDecl.
//
// Given
-// struct MyStrict {
+// struct MyStruct {
// int f;
// int f2, f3;
+// struct S { int x } f4;
// };
-// matches |int f|, but does not match declarations of |f2| and |f3|.
-AST_MATCHER(clang::FieldDecl, hasUniqueTypeLoc) {
+// - doesn't match |f|
+// - matches |f2| and |f3| (which overlap each other's location)
+// - matches |f4| (which overlaps the location of |S|)
+AST_MATCHER(clang::FieldDecl, overlapsOtherDeclsWithinRecordDecl) {
const clang::FieldDecl& self = Node;
+ const clang::SourceManager& source_manager =
+ Finder->getASTContext().getSourceManager();
+
const clang::RecordDecl* record_decl = self.getParent();
- clang::SourceLocation self_type_loc =
- self.getTypeSourceInfo()->getTypeLoc().getBeginLoc();
+ clang::SourceRange self_range(self.getBeginLoc(), self.getEndLoc());
- bool has_sibling_with_same_type_loc =
- std::any_of(record_decl->field_begin(), record_decl->field_end(),
- [&](const clang::FieldDecl* f) {
- // Is |f| a real sibling?
- if (f == &self)
- return false; // Not a sibling.
+ auto is_overlapping_sibling = [&](const clang::Decl* other_decl) {
+ if (other_decl == &self)
+ return false;
- clang::SourceLocation sibling_type_loc =
- f->getTypeSourceInfo()->getTypeLoc().getBeginLoc();
- return self_type_loc == sibling_type_loc;
- });
-
- return !has_sibling_with_same_type_loc;
+ clang::SourceRange other_range(other_decl->getBeginLoc(),
+ other_decl->getEndLoc());
+ return IsOverlapping(source_manager, self_range, other_range);
+ };
+ bool has_sibling_with_overlapping_location =
+ std::any_of(record_decl->decls_begin(), record_decl->decls_end(),
+ is_overlapping_sibling);
+ return has_sibling_with_overlapping_location;
}
// Rewrites |SomeClass* field| (matched as "affectedFieldDecl") into
@@ -631,13 +679,10 @@
auto record_with_deleted_allocation_operator_type_matcher =
recordType(hasDeclaration(cxxRecordDecl(
hasMethod(allOf(hasOverloadedOperatorName("new"), isDeleted())))));
- auto non_free_standing_tag_type =
- tagType(hasDeclaration(tagDecl(isNotFreeStandingTagDecl())));
auto supported_pointer_types_matcher =
- pointerType(unless(pointee(hasUnqualifiedDesugaredType(
- anyOf(record_with_deleted_allocation_operator_type_matcher,
- non_free_standing_tag_type, functionType(), memberPointerType(),
- anyCharType(), arrayType())))));
+ pointerType(unless(pointee(hasUnqualifiedDesugaredType(anyOf(
+ record_with_deleted_allocation_operator_type_matcher, functionType(),
+ memberPointerType(), anyCharType(), arrayType())))));
// Implicit field declarations =========
// Matches field declarations that do not explicitly appear in the source
@@ -669,8 +714,9 @@
FieldDeclFilterFile fields_to_exclude(exclude_fields_param);
auto field_decl_matcher =
fieldDecl(
- allOf(hasType(supported_pointer_types_matcher), hasUniqueTypeLoc(),
- unless(anyOf(isInThirdPartyLocation(), isInGeneratedLocation(),
+ allOf(hasType(supported_pointer_types_matcher),
+ unless(anyOf(overlapsOtherDeclsWithinRecordDecl(),
+ isInThirdPartyLocation(), isInGeneratedLocation(),
isExpansionInSystemHeader(), isInMacroLocation(),
isInExternCContext(),
isListedInFilterFile(fields_to_exclude),
diff --git a/tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc b/tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc
index 73f5366..4303de79 100644
--- a/tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc
+++ b/tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc
@@ -5,6 +5,9 @@
#include <stddef.h>
#include <stdint.h>
+#include <string>
+#include <vector>
+
#include "base/memory/checked_ptr.h"
namespace my_namespace {
@@ -15,6 +18,11 @@
int data_member;
};
+template <typename T>
+struct SomeTemplate {
+ T t;
+};
+
// The class below deletes the |operator new| - this simulate's Blink's
// STACK_ALLOCATED macro and/or OilPan / GarbageCollected<T> classes.
class NoNewOperator {
@@ -40,6 +48,14 @@
// Expected rewrite: CheckedPtr<const bool> bool_ptr;
CheckedPtr<const bool> const_bool_ptr;
+ // Pointers to templates.
+ // Expected rewrite: CheckedPtr<std::string> string_ptr;
+ CheckedPtr<std::string> string_ptr;
+ // Expected rewrite: CheckedPtr<std::vector<char>> vector_ptr;
+ CheckedPtr<std::vector<char>> vector_ptr;
+ // Expected rewrite: CheckedPtr<SomeTemplate<char>> template_ptr;
+ CheckedPtr<SomeTemplate<char>> template_ptr;
+
// Some types may be spelled in various, alternative ways. If possible, the
// rewriter should preserve the original spelling.
//
diff --git a/tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc b/tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc
index b1a3eaef..0e8659a 100644
--- a/tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc
+++ b/tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc
@@ -5,6 +5,9 @@
#include <stddef.h>
#include <stdint.h>
+#include <string>
+#include <vector>
+
namespace my_namespace {
class SomeClass {
@@ -13,6 +16,11 @@
int data_member;
};
+template <typename T>
+struct SomeTemplate {
+ T t;
+};
+
// The class below deletes the |operator new| - this simulate's Blink's
// STACK_ALLOCATED macro and/or OilPan / GarbageCollected<T> classes.
class NoNewOperator {
@@ -38,6 +46,14 @@
// Expected rewrite: CheckedPtr<const bool> bool_ptr;
const bool* const_bool_ptr;
+ // Pointers to templates.
+ // Expected rewrite: CheckedPtr<std::string> string_ptr;
+ std::string* string_ptr;
+ // Expected rewrite: CheckedPtr<std::vector<char>> vector_ptr;
+ std::vector<char>* vector_ptr;
+ // Expected rewrite: CheckedPtr<SomeTemplate<char>> template_ptr;
+ SomeTemplate<char>* template_ptr;
+
// Some types may be spelled in various, alternative ways. If possible, the
// rewriter should preserve the original spelling.
//
