Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following rules:
1. relative targets (":a", "../b") sort before absolute targets ("//c"),
2. each group (relative and absolute) is sorted alphabetically.
BUG=554928
Review URL: https://codereview.chromium.org/1442023002
Cr-Original-Commit-Position: refs/heads/master@{#360042}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 354261e7dd491e88fde3b3bd3fa5ef6f1c629f00
diff --git a/tools/gn/command_format.cc b/tools/gn/command_format.cc
index 5eff601..6c199d3 100644
--- a/tools/gn/command_format.cc
+++ b/tools/gn/command_format.cc
@@ -128,8 +128,11 @@
// Flag assignments to sources, deps, etc. to make their RHSs multiline.
void AnnotatePreferredMultilineAssignment(const BinaryOpNode* binop);
- // Alphabetically a list on the RHS if the LHS is 'sources'.
- void SortIfSources(const BinaryOpNode* binop);
+ // Sort a list on the RHS if the LHS is 'sources', 'deps' or 'public_deps'.
+ // The 'sources' are sorted alphabetically while the 'deps' and 'public_deps'
+ // are sorted putting first the relative targets and then the global ones
+ // (both sorted alphabetically).
+ void SortIfSourcesOrDeps(const BinaryOpNode* binop);
// Heuristics to decide if there should be a blank line added between two
// items. For various "small" items, it doesn't look nice if there's too much
@@ -307,20 +310,20 @@
}
}
-void Printer::SortIfSources(const BinaryOpNode* binop) {
+void Printer::SortIfSourcesOrDeps(const BinaryOpNode* binop) {
const IdentifierNode* ident = binop->left()->AsIdentifier();
const ListNode* list = binop->right()->AsList();
- // TODO(scottmg): Sort more than 'sources'?
if ((binop->op().value() == "=" || binop->op().value() == "+=" ||
binop->op().value() == "-=") &&
ident && list) {
const base::StringPiece lhs = ident->value().value();
if (lhs == "sources")
const_cast<ListNode*>(list)->SortAsStringsList();
+ else if (lhs == "deps" || lhs == "public_deps")
+ const_cast<ListNode*>(list)->SortAsDepsList();
}
}
-
bool Printer::ShouldAddBlankLineInBetween(const ParseNode* a,
const ParseNode* b) {
LocationRange a_range = a->GetRange();
@@ -458,7 +461,7 @@
CHECK(precedence_.find(binop->op().value()) != precedence_.end());
AnnotatePreferredMultilineAssignment(binop);
- SortIfSources(binop);
+ SortIfSourcesOrDeps(binop);
Precedence prec = precedence_[binop->op().value()];
diff --git a/tools/gn/command_format_unittest.cc b/tools/gn/command_format_unittest.cc
index e0f6816..8e8c256 100644
--- a/tools/gn/command_format_unittest.cc
+++ b/tools/gn/command_format_unittest.cc
@@ -100,3 +100,6 @@
FORMAT_TEST(060)
FORMAT_TEST(061)
FORMAT_TEST(062)
+FORMAT_TEST(063)
+FORMAT_TEST(064)
+FORMAT_TEST(065)
diff --git a/tools/gn/format_test_data/063.gn b/tools/gn/format_test_data/063.gn
new file mode 100644
index 0000000..a933f4b
--- /dev/null
+++ b/tools/gn/format_test_data/063.gn
@@ -0,0 +1,18 @@
+source_set("test") {
+ deps = [
+ "//c",
+ "//a",
+ ":b",
+ "../d",
+ ]
+
+ public_deps = []
+ if (condition) {
+ public_deps += [
+ "//y",
+ "//w",
+ ":x",
+ "../z",
+ ]
+ }
+}
diff --git a/tools/gn/format_test_data/063.golden b/tools/gn/format_test_data/063.golden
new file mode 100644
index 0000000..acf2843
--- /dev/null
+++ b/tools/gn/format_test_data/063.golden
@@ -0,0 +1,18 @@
+source_set("test") {
+ deps = [
+ ":b",
+ "../d",
+ "//a",
+ "//c",
+ ]
+
+ public_deps = []
+ if (condition) {
+ public_deps += [
+ ":x",
+ "../z",
+ "//w",
+ "//y",
+ ]
+ }
+}
diff --git a/tools/gn/format_test_data/064.gn b/tools/gn/format_test_data/064.gn
new file mode 100644
index 0000000..c186725
--- /dev/null
+++ b/tools/gn/format_test_data/064.gn
@@ -0,0 +1,3 @@
+source_set("test") {
+ deps = [ rebase_path(sdk_dep, ".", mojo_root) ]
+}
diff --git a/tools/gn/format_test_data/064.golden b/tools/gn/format_test_data/064.golden
new file mode 100644
index 0000000..3ff56f6
--- /dev/null
+++ b/tools/gn/format_test_data/064.golden
@@ -0,0 +1,5 @@
+source_set("test") {
+ deps = [
+ rebase_path(sdk_dep, ".", mojo_root),
+ ]
+}
diff --git a/tools/gn/format_test_data/065.gn b/tools/gn/format_test_data/065.gn
new file mode 100644
index 0000000..a448909
--- /dev/null
+++ b/tools/gn/format_test_data/065.gn
@@ -0,0 +1,4 @@
+source_set("test") {
+ some_target_name = ":some_target"
+ deps = [ some_target_name, "//last_target", ":another_target" ]
+}
diff --git a/tools/gn/format_test_data/065.golden b/tools/gn/format_test_data/065.golden
new file mode 100644
index 0000000..5df85fd
--- /dev/null
+++ b/tools/gn/format_test_data/065.golden
@@ -0,0 +1,8 @@
+source_set("test") {
+ some_target_name = ":some_target"
+ deps = [
+ ":another_target",
+ "//last_target",
+ some_target_name,
+ ]
+}
diff --git a/tools/gn/parse_tree.cc b/tools/gn/parse_tree.cc
index 5e5b9ea..d22adca 100644
--- a/tools/gn/parse_tree.cc
+++ b/tools/gn/parse_tree.cc
@@ -5,6 +5,7 @@
#include "tools/gn/parse_tree.h"
#include <string>
+#include <tuple>
#include "base/stl_util.h"
#include "base/strings/string_number_conversions.h"
@@ -15,6 +16,20 @@
namespace {
+std::tuple<base::StringPiece, base::StringPiece> SplitAtFirst(
+ base::StringPiece str,
+ char c) {
+ if (!str.starts_with("\"") || !str.ends_with("\""))
+ return std::make_tuple(str, base::StringPiece());
+
+ str = str.substr(1, str.length() - 2);
+ size_t index_of_first = str.find(c);
+ return std::make_tuple(str.substr(0, index_of_first),
+ index_of_first != base::StringPiece::npos
+ ? str.substr(index_of_first + 1)
+ : base::StringPiece());
+}
+
std::string IndentFor(int value) {
return std::string(value, ' ');
}
@@ -502,10 +517,21 @@
end_->Print(out, indent + 1);
}
-void ListNode::SortAsStringsList() {
- // Sorts alphabetically. Partitions first on BlockCommentNodes and sorts each
- // partition separately.
+template <typename Comparator>
+void ListNode::SortList(Comparator comparator) {
+ // Partitions first on BlockCommentNodes and sorts each partition separately.
for (auto sr : GetSortRanges()) {
+ bool skip = false;
+ for (size_t i = sr.begin; i != sr.end; ++i) {
+ // Bails out if any of the nodes are unsupported.
+ const ParseNode* node = contents_[i];
+ if (!node->AsLiteral() && !node->AsIdentifier() && !node->AsAccessor()) {
+ skip = true;
+ continue;
+ }
+ }
+ if (skip)
+ continue;
// Save the original line number so that we can re-assign ranges. We assume
// they're contiguous lines because GetSortRanges() does so above. We need
// to re-assign these line numbers primiarily because `gn format` uses them
@@ -514,11 +540,7 @@
int start_line = contents_[sr.begin]->GetRange().begin().line_number();
const ParseNode* original_first = contents_[sr.begin];
std::sort(contents_.begin() + sr.begin, contents_.begin() + sr.end,
- [](const ParseNode* a, const ParseNode* b) {
- base::StringPiece astr = GetStringRepresentation(a);
- base::StringPiece bstr = GetStringRepresentation(b);
- return astr < bstr;
- });
+ comparator);
// If the beginning of the range had before comments, and the first node
// moved during the sort, then move its comments to the new head of the
// range.
@@ -553,6 +575,25 @@
}
}
+void ListNode::SortAsStringsList() {
+ // Sorts alphabetically.
+ SortList([](const ParseNode* a, const ParseNode* b) {
+ base::StringPiece astr = GetStringRepresentation(a);
+ base::StringPiece bstr = GetStringRepresentation(b);
+ return astr < bstr;
+ });
+}
+
+void ListNode::SortAsDepsList() {
+ // Sorts first relative targets, then absolute, each group is sorted
+ // alphabetically.
+ SortList([](const ParseNode* a, const ParseNode* b) {
+ base::StringPiece astr = GetStringRepresentation(a);
+ base::StringPiece bstr = GetStringRepresentation(b);
+ return SplitAtFirst(astr, ':') < SplitAtFirst(bstr, ':');
+ });
+}
+
// 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/tools/gn/parse_tree.h b/tools/gn/parse_tree.h
index b134d4a..36c511a 100644
--- a/tools/gn/parse_tree.h
+++ b/tools/gn/parse_tree.h
@@ -372,6 +372,7 @@
const std::vector<const ParseNode*>& contents() const { return contents_; }
void SortAsStringsList();
+ void SortAsDepsList();
// During formatting, do we want this list to always be multliline? This is
// used to make assignments to deps, sources, etc. always be multiline lists,
@@ -390,6 +391,9 @@
std::vector<SortRange> GetSortRanges() const;
private:
+ template <typename Comparator>
+ void SortList(Comparator comparator);
+
// Tokens corresponding to the [ and ]. The end token is stored in inside an
// custom parse node so that it can have comments hung off of it.
Token begin_token_;
