Deduplicate item in 'deps', 'sources' and related lists
When sorting 'deps', 'sources' and related lists, also remove any
duplicated items (this can be inhibited by using a "# KEEPDUPS"
comment before the list).
Also fix the sorting of accessor nodes (they were ignoring the
member or subscript part when comparing them).
Fixed: 42440318
Change-Id: I99aae1c2dc2008c21423464e0744792ada96732c
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19660
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
diff --git a/src/gn/command_format.cc b/src/gn/command_format.cc
index a509eed..9805309 100644
--- a/src/gn/command_format.cc
+++ b/src/gn/command_format.cc
@@ -124,6 +124,10 @@
return ident.ends_with("deps") || ident == "visibility";
}
+bool IsSourcesList(std::string_view ident) {
+ return ident.ends_with("sources") || ident == "public";
+}
+
class Printer {
public:
Printer();
@@ -182,6 +186,10 @@
// 'visibility': same as 'deps'.
void SortIfApplicable(const BinaryOpNode* binop);
+ // Remove duplicates. Applies to 'visibility', 'deps' or ends in 'deps',
+ // 'sources' or ends in 'sources', or 'public'.
+ void DeduplicateIfApplicable(const BinaryOpNode* binop);
+
// Traverse a binary op node tree and apply a callback to each leaf node.
void TraverseBinaryOpNode(const ParseNode* node,
std::function<void(const ParseNode*)> callback);
@@ -442,7 +450,7 @@
}
const std::string_view lhs = ident->value().value();
- if (lhs.ends_with("sources") || lhs == "public") {
+ if (IsSourcesList(lhs)) {
TraverseBinaryOpNode(binop->right(), [](const ParseNode* node) {
const ListNode* list = node->AsList();
if (list)
@@ -457,6 +465,31 @@
}
}
+void Printer::DeduplicateIfApplicable(const BinaryOpNode* binop) {
+ if (const Comments* comments = binop->comments()) {
+ const std::vector<Token>& before = comments->before();
+ if (!before.empty() && (before.front().value() == "# KEEPDUPS" ||
+ before.back().value() == "# KEEPDUPS")) {
+ // Allow disabling of deduplication for specific actions that might
+ // want duplicate sources.
+ return;
+ }
+ }
+ const IdentifierNode* ident = binop->left()->AsIdentifier();
+ if (!ident || !IsAssignment(binop->op().value())) {
+ return;
+ }
+
+ const std::string_view lhs = ident->value().value();
+ if (IsSourcesList(lhs) || IsTargetsList(lhs)) {
+ TraverseBinaryOpNode(binop->right(), [](const ParseNode* node) {
+ const ListNode* list = node->AsList();
+ if (list)
+ const_cast<ListNode*>(list)->DeduplicateList();
+ });
+ }
+}
+
void Printer::TraverseBinaryOpNode(
const ParseNode* node,
std::function<void(const ParseNode*)> callback) {
@@ -790,6 +823,7 @@
// Shorten before sorting, since the shortening may affect the ordering.
ShortenIfApplicable(binop);
SortIfApplicable(binop);
+ DeduplicateIfApplicable(binop);
Precedence prec = precedence_[binop->op().value()];
diff --git a/src/gn/command_format_unittest.cc b/src/gn/command_format_unittest.cc
index de7e004..645ec27 100644
--- a/src/gn/command_format_unittest.cc
+++ b/src/gn/command_format_unittest.cc
@@ -137,3 +137,4 @@
FORMAT_TEST(082)
FORMAT_TEST(083)
FORMAT_TEST(084)
+FORMAT_TEST(085)
diff --git a/src/gn/format_test_data/063.gn b/src/gn/format_test_data/063.gn
index de5002e..2a5659f 100644
--- a/src/gn/format_test_data/063.gn
+++ b/src/gn/format_test_data/063.gn
@@ -1,6 +1,15 @@
source_set("test") {
a = "a"
b = "b"
+ c = {
+ d = "d"
+ e = "e"
+ }
+ f = [
+ "f0",
+ "f1",
+ "f2",
+ ]
deps = [
"//a",
"//a/a",
@@ -8,8 +17,12 @@
"//a:a",
"//a:b",
"//b",
+ c.e,
+ c.d,
":a",
":b",
+ f[2],
+ f[0],
"a",
"a/a",
"a/b",
@@ -18,6 +31,9 @@
"b",
a,
b,
+ c.d,
+ f[0],
+ f[1],
]
public_deps = []
@@ -32,6 +48,18 @@
"a:a",
a,
]
+
+ deps += [
+ ":d",
+ ":e",
+ # Some comment attached to an item. Should be moved with the item
+ # when it is sorted.
+ ":a",
+ ":b",
+ # Comment attached to a duplicated item. Should be concatenated to
+ # the comment from the other item.
+ ":a",
+ ]
}
# Sort lists with "deps" suffix as "deps".
diff --git a/src/gn/format_test_data/063.golden b/src/gn/format_test_data/063.golden
index 73cfc04..3c0d677 100644
--- a/src/gn/format_test_data/063.golden
+++ b/src/gn/format_test_data/063.golden
@@ -1,23 +1,35 @@
source_set("test") {
a = "a"
b = "b"
+ c = {
+ d = "d"
+ e = "e"
+ }
+ f = [
+ "f0",
+ "f1",
+ "f2",
+ ]
deps = [
":a",
":b",
"a",
- "a",
"a:b",
"a/a",
"a/b",
"b",
"//a",
- "//a",
"//a:b",
"//a/a",
"//a/b",
"//b",
a,
b,
+ c.d,
+ c.e,
+ f[0],
+ f[1],
+ f[2],
]
public_deps = []
@@ -25,13 +37,22 @@
public_deps += [
":a",
"a",
- "a",
"a/a",
"//a",
- "//a",
"//a/a",
a,
]
+
+ deps += [
+ # Some comment attached to an item. Should be moved with the item
+ # when it is sorted.
+ # Comment attached to a duplicated item. Should be concatenated to
+ # the comment from the other item.
+ ":a",
+ ":b",
+ ":d",
+ ":e",
+ ]
}
# Sort lists with "deps" suffix as "deps".
diff --git a/src/gn/format_test_data/085.gn b/src/gn/format_test_data/085.gn
new file mode 100644
index 0000000..c8ada36
--- /dev/null
+++ b/src/gn/format_test_data/085.gn
@@ -0,0 +1,32 @@
+# Suppress deduplication based on comment.
+
+# KEEPDUPS
+sources = []
+
+# KEEPDUPS
+sources = [ "a" ]
+
+# KEEPDUPS
+sources += [ "a" ]
+
+# KEEPDUPS
+# This is KEEPDUPS because reason.
+sources = [
+ "z",
+ "a",
+ "z",
+ "z2",
+ "a",
+ "y.cc",
+]
+
+# This is KEEPDUPS because reason:
+# KEEPDUPS
+sources += [
+ "z",
+ "a",
+ "z",
+ "z2",
+ "a",
+ "y.cc",
+]
diff --git a/src/gn/format_test_data/085.golden b/src/gn/format_test_data/085.golden
new file mode 100644
index 0000000..7c5dde5
--- /dev/null
+++ b/src/gn/format_test_data/085.golden
@@ -0,0 +1,32 @@
+# Suppress deduplication based on comment.
+
+# KEEPDUPS
+sources = []
+
+# KEEPDUPS
+sources = [ "a" ]
+
+# KEEPDUPS
+sources += [ "a" ]
+
+# KEEPDUPS
+# This is KEEPDUPS because reason.
+sources = [
+ "a",
+ "a",
+ "y.cc",
+ "z",
+ "z",
+ "z2",
+]
+
+# This is KEEPDUPS because reason:
+# KEEPDUPS
+sources += [
+ "a",
+ "a",
+ "y.cc",
+ "z",
+ "z",
+ "z2",
+]
diff --git a/src/gn/parse_tree.cc b/src/gn/parse_tree.cc
index b9f04f1..e4fba38 100644
--- a/src/gn/parse_tree.cc
+++ b/src/gn/parse_tree.cc
@@ -7,6 +7,7 @@
#include <stdint.h>
#include <memory>
+#include <sstream>
#include <string>
#include <tuple>
@@ -83,15 +84,24 @@
static_cast<int>(node->comments()->before().size() + 1)));
}
-std::string_view GetStringRepresentation(const ParseNode* node) {
+std::string GetStringRepresentation(const ParseNode* node) {
DCHECK(node->AsLiteral() || node->AsIdentifier() || node->AsAccessor());
if (node->AsLiteral())
- return node->AsLiteral()->value().value();
- else if (node->AsIdentifier())
- return node->AsIdentifier()->value().value();
- else if (node->AsAccessor())
- return node->AsAccessor()->base().value();
- return std::string_view();
+ return std::string(node->AsLiteral()->value().value());
+ if (node->AsIdentifier())
+ return std::string(node->AsIdentifier()->value().value());
+ if (const auto* accessor_node = node->AsAccessor()) {
+ std::ostringstream buffer;
+ buffer << accessor_node->base().value();
+ if (const auto* subscript_node = accessor_node->subscript()) {
+ buffer << "[" << GetStringRepresentation(subscript_node) << "]";
+ }
+ if (const auto* member_node = accessor_node->member()) {
+ buffer << "." << member_node->value().value();
+ }
+ return buffer.str();
+ }
+ return std::string();
}
void AddLocationJSONNodes(base::Value* dict, LocationRange location) {
@@ -156,6 +166,18 @@
value.FindKey(kJsonNodeValue)->GetString());
}
+void SetLineNumber(const ParseNode* node, int line_number) {
+ DCHECK(node->AsLiteral() || node->AsIdentifier() || node->AsAccessor());
+ if (node->AsLiteral()) {
+ const_cast<LiteralNode*>(node->AsLiteral())->SetNewLocation(line_number);
+ } else if (node->AsIdentifier()) {
+ const_cast<IdentifierNode*>(node->AsIdentifier())
+ ->SetNewLocation(line_number);
+ } else if (node->AsAccessor()) {
+ const_cast<AccessorNode*>(node->AsAccessor())->SetNewLocation(line_number);
+ }
+}
+
} // namespace
Comments::Comments() = default;
@@ -978,19 +1000,8 @@
const ParseNode* prev = nullptr;
for (size_t i = sr.begin; i != sr.end; ++i) {
const ParseNode* node = contents_[i].get();
- DCHECK(node->AsLiteral() || node->AsIdentifier() || node->AsAccessor());
- int line_number =
- prev ? prev->GetRange().end().line_number() + 1 : start_line;
- if (node->AsLiteral()) {
- const_cast<LiteralNode*>(node->AsLiteral())
- ->SetNewLocation(line_number);
- } else if (node->AsIdentifier()) {
- const_cast<IdentifierNode*>(node->AsIdentifier())
- ->SetNewLocation(line_number);
- } else if (node->AsAccessor()) {
- const_cast<AccessorNode*>(node->AsAccessor())
- ->SetNewLocation(line_number);
- }
+ SetLineNumber(
+ node, prev ? prev->GetRange().end().line_number() + 1 : start_line);
prev = node;
}
}
@@ -1007,8 +1018,8 @@
void ListNode::SortAsStringsList() {
// Sorts alphabetically.
SortList([](const ParseNode* a, const ParseNode* b) {
- std::string_view astr = GetStringRepresentation(a);
- std::string_view bstr = GetStringRepresentation(b);
+ std::string astr = GetStringRepresentation(a);
+ std::string bstr = GetStringRepresentation(b);
return astr < bstr;
});
}
@@ -1017,13 +1028,59 @@
// Sorts first relative targets, then absolute, each group is sorted
// alphabetically.
SortList([](const ParseNode* a, const ParseNode* b) {
- std::string_view astr = GetStringRepresentation(a);
- std::string_view bstr = GetStringRepresentation(b);
+ std::string astr = GetStringRepresentation(a);
+ std::string bstr = GetStringRepresentation(b);
return std::make_pair(GetDepsCategory(astr), SplitAtFirst(astr, ':')) <
std::make_pair(GetDepsCategory(bstr), SplitAtFirst(bstr, ':'));
});
}
+void ListNode::DeduplicateList() {
+ // Nothing to do if the list is empty.
+ if (contents_.empty()) {
+ return;
+ }
+
+ // First check if there is any unsupported nodes and bail out if this
+ // is the case.
+ for (auto& node : contents_) {
+ if (!node->AsLiteral() && !node->AsIdentifier() && !node->AsAccessor())
+ return;
+ }
+
+ const ParseNode* prev = nullptr;
+ auto write_iter = contents_.begin();
+ std::map<std::string, const ParseNode*> seen;
+
+ for (auto& node : contents_) {
+ std::string item_repr = GetStringRepresentation(node.get());
+ if (auto iter = seen.find(item_repr); iter != seen.end()) {
+ // If the node has any comment, move them to the item it is a
+ // duplicate of.
+ if (node->comments()) {
+ for (const auto& hc : node->comments()->before()) {
+ const_cast<ParseNode*>(iter->second)
+ ->comments_mutable()
+ ->append_before(hc);
+ }
+ }
+ continue;
+ }
+
+ seen.emplace(std::move(item_repr), node.get());
+ if (prev) {
+ SetLineNumber(node.get(), prev->GetRange().end().line_number() + 1);
+ }
+
+ prev = node.get();
+ *write_iter++ = std::move(node);
+ }
+
+ if (write_iter != contents_.end()) {
+ contents_.erase(write_iter, contents_.end());
+ }
+}
+
// Breaks the ParseNodes of |contents| up by ranges that should be separately
// sorted. In particular, we break at a block comment, or an item that has an
// attached "before" comment and is separated by a blank line from the item
diff --git a/src/gn/parse_tree.h b/src/gn/parse_tree.h
index 6ad2aec..f3aeb58 100644
--- a/src/gn/parse_tree.h
+++ b/src/gn/parse_tree.h
@@ -471,6 +471,7 @@
void ShortenTargets();
void SortAsStringsList();
void SortAsTargetsList();
+ void DeduplicateList();
struct SortRange {
size_t begin;
