Change the way hard splits are handled.
By having a separate rule for hard splits, we lose any constraints the
original soft rule may have had or -- more importantly -- other rules
may have had on it.
For constraints where hardening the rule forces other rules to fully
harden, it's fine because we propagate those constraints when the rule
is hardened. The problem comes with "cannot split" constraints. If rule
A has a "cannot split" constraint on rule B when rule A has some value,
we need to remember that even if B is hardened so that we prevent A
from taking that value.
Instead of swapping out the rule with a separate hard split rule, the
idea is to keep the rule (and thus preserve references to it) but change
it to a "hardened" state.
R=kevmoo@google.com
Review URL: https://codereview.chromium.org//1492683002 .
diff --git a/lib/src/argument_list_visitor.dart b/lib/src/argument_list_visitor.dart
index e8d3d8a..2fb8e97 100644
--- a/lib/src/argument_list_visitor.dart
+++ b/lib/src/argument_list_visitor.dart
@@ -290,7 +290,7 @@
void visit(SourceVisitor visitor) {
if (_collections.isNotEmpty) {
- _collectionRule = new SimpleRule(Cost.splitCollections);
+ _collectionRule = new Rule(Cost.splitCollections);
}
var rule = _visitPositional(visitor);
diff --git a/lib/src/chunk.dart b/lib/src/chunk.dart
index a382501..da6d7bb 100644
--- a/lib/src/chunk.dart
+++ b/lib/src/chunk.dart
@@ -114,10 +114,6 @@
Rule get rule => _rule;
Rule _rule;
- /// Whether this chunk is always followed by a newline or whether the line
- /// splitter may choose to keep the next chunk on the same line.
- bool get isHardSplit => _rule is HardSplitRule;
-
/// Whether or not an extra blank line should be output after this chunk if
/// it's split.
///
@@ -197,15 +193,6 @@
_text += text;
}
- /// Forces this soft split to become a hard split.
- ///
- /// This is called on the soft splits owned by a rule that decides to harden
- /// when it finds out another hard split occurs within its chunks.
- void harden() {
- _rule = new HardSplitRule();
- spans.clear();
- }
-
/// Finishes off this chunk with the given [rule] and split information.
///
/// This may be called multiple times on the same split since the splits
@@ -216,7 +203,7 @@
{bool flushLeft, bool isDouble, bool space}) {
if (flushLeft == null) flushLeft = false;
if (space == null) space = false;
- if (isHardSplit || rule is HardSplitRule) {
+ if (rule.isHardened) {
// A hard split always wins.
_rule = rule;
} else if (_rule == null) {
@@ -271,10 +258,9 @@
if (_rule == null) {
parts.add("(no split)");
- } else if (isHardSplit) {
- parts.add("hard");
} else {
parts.add(rule.toString());
+ if (rule.isHardened) parts.add("(hard)");
if (_rule.constrainedRules.isNotEmpty) {
parts.add("-> ${_rule.constrainedRules.join(' ')}");
diff --git a/lib/src/chunk_builder.dart b/lib/src/chunk_builder.dart
index 83f62a4..c70b0e1 100644
--- a/lib/src/chunk_builder.dart
+++ b/lib/src/chunk_builder.dart
@@ -361,15 +361,15 @@
var span = new Span(openSpan.cost);
for (var i = openSpan.start; i < end; i++) {
var chunk = _chunks[i];
- if (!chunk.isHardSplit) chunk.spans.add(span);
+ if (!chunk.rule.isHardened) chunk.spans.add(span);
}
}
/// Starts a new [Rule].
///
- /// If omitted, defaults to a new [SimpleRule].
+ /// If omitted, defaults to a new [Rule].
void startRule([Rule rule]) {
- if (rule == null) rule = new SimpleRule();
+ if (rule == null) rule = new Rule();
// See if any of the rules that contain this one care if it splits.
_rules.forEach((outer) => outer.contain(rule));
@@ -383,9 +383,9 @@
/// first operand but not get forced to split if a comment appears before the
/// entire expression.
///
- /// If [rule] is omitted, defaults to a new [SimpleRule].
+ /// If [rule] is omitted, defaults to a new [Rule].
void startLazyRule([Rule rule]) {
- if (rule == null) rule = new SimpleRule();
+ if (rule == null) rule = new Rule();
_lazyRules.add(rule);
}
@@ -490,7 +490,7 @@
/// `true`, the block is considered to always split.
///
/// Returns the previous writer for the surrounding block.
- ChunkBuilder endBlock(HardSplitRule ignoredSplit, {bool forceSplit}) {
+ ChunkBuilder endBlock(Rule ignoredSplit, {bool forceSplit}) {
_divideChunks();
// If we don't already know if the block is going to split, see if it
@@ -504,7 +504,9 @@
break;
}
- if (chunk.isHardSplit && chunk.rule != ignoredSplit) {
+ if (chunk.rule != null &&
+ chunk.rule.isHardened &&
+ chunk.rule != ignoredSplit) {
forceSplit = true;
break;
}
@@ -682,7 +684,7 @@
void _writeHardSplit({bool isDouble, bool flushLeft, bool nest: false}) {
// A hard split overrides any other whitespace.
_pendingWhitespace = null;
- _writeSplit(new HardSplitRule(),
+ _writeSplit(new Rule.hard(),
flushLeft: flushLeft, isDouble: isDouble, nest: nest);
}
@@ -711,11 +713,11 @@
Chunk _afterSplit() {
var chunk = _chunks.last;
- if (chunk.rule is! HardSplitRule) {
+ if (_rules.isNotEmpty) {
_ruleChunks.putIfAbsent(rule, () => []).add(_chunks.length - 1);
}
- if (chunk.isHardSplit) _handleHardSplit();
+ if (chunk.rule.isHardened) _handleHardSplit();
return chunk;
}
@@ -733,8 +735,11 @@
/// Returns true if we can divide the chunks at [index] and line split the
/// ones before and after that separately.
bool _canDivideAt(int i) {
+ // Don't divide after the last chunk.
+ if (i == _chunks.length - 1) return false;
+
var chunk = _chunks[i];
- if (!chunk.isHardSplit) return false;
+ if (!chunk.rule.isHardened) return false;
if (chunk.nesting.isNested) return false;
if (chunk.isBlock) return false;
@@ -782,18 +787,16 @@
void _hardenRules() {
if (_hardSplitRules.isEmpty) return;
- // Harden all of the rules that are constrained by [rules] as well.
- var hardenedRules = new Set();
walkConstraints(rule) {
- if (hardenedRules.contains(rule)) return;
- hardenedRules.add(rule);
+ rule.harden();
// Follow this rule's constraints, recursively.
for (var other in _ruleChunks.keys) {
if (other == rule) continue;
- if (rule.constrain(rule.fullySplitValue, other) ==
- other.fullySplitValue) {
+ if (!other.isHardened &&
+ rule.constrain(rule.fullySplitValue, other) ==
+ other.fullySplitValue) {
walkConstraints(other);
}
}
@@ -803,10 +806,11 @@
walkConstraints(rule);
}
- // Harden every chunk that uses one of these rules.
+ // Discard spans in hardened chunks since we know for certain they will
+ // split anyway.
for (var chunk in _chunks) {
- if (hardenedRules.contains(chunk.rule)) {
- chunk.harden();
+ if (chunk.rule != null && chunk.rule.isHardened) {
+ chunk.spans.clear();
}
}
}
diff --git a/lib/src/debug.dart b/lib/src/debug.dart
index 77590c0..60cb5f7 100644
--- a/lib/src/debug.dart
+++ b/lib/src/debug.dart
@@ -84,10 +84,8 @@
spans = spans.toList();
- var rules = chunks
- .map((chunk) => chunk.rule)
- .where((rule) => rule != null && rule is! HardSplitRule)
- .toSet();
+ var rules =
+ chunks.map((chunk) => chunk.rule).where((rule) => rule != null).toSet();
var rows = [];
@@ -132,13 +130,12 @@
row.add("");
row.add("(no rule)");
row.add("");
- } else if (chunk.isHardSplit) {
- row.add("");
- row.add("(hard)");
- row.add("");
- } else if (chunk.rule != null) {
+ } else {
writeIf(chunk.rule.cost != 0, () => "\$${chunk.rule.cost}");
- row.add(chunk.rule.toString());
+
+ var ruleString = chunk.rule.toString();
+ if (chunk.rule.isHardened) ruleString += "!";
+ row.add(ruleString);
var constrainedRules =
chunk.rule.constrainedRules.toSet().intersection(rules);
@@ -191,6 +188,33 @@
print(buffer.toString());
}
+/// Shows all of the constraints between the rules used by [chunks].
+void dumpConstraints(List<Chunk> chunks) {
+ var rules =
+ chunks.map((chunk) => chunk.rule).where((rule) => rule != null).toSet();
+
+ for (var rule in rules) {
+ var constrainedValues = [];
+ for (var value = 0; value < rule.numValues; value++) {
+ var constraints = [];
+ for (var other in rules) {
+ if (rule == other) continue;
+
+ var constraint = rule.constrain(value, other);
+ if (constraint != null) {
+ constraints.add("$other->$constraint");
+ }
+ }
+
+ if (constraints.isNotEmpty) {
+ constrainedValues.add("$value:(${constraints.join(' ')})");
+ }
+ }
+
+ log("$rule ${constrainedValues.join(' ')}");
+ }
+}
+
/// Convert the line to a [String] representation.
///
/// It will determine how best to split it into multiple lines of output and
diff --git a/lib/src/line_splitting/line_splitter.dart b/lib/src/line_splitting/line_splitter.dart
index 78aa54d..420c509 100644
--- a/lib/src/line_splitting/line_splitter.dart
+++ b/lib/src/line_splitting/line_splitter.dart
@@ -130,10 +130,10 @@
int firstLineIndent,
{bool flushLeft: false})
: chunks = chunks,
- // Collect the set of soft rules that we need to select values for.
+ // Collect the set of rules that we need to select values for.
rules = chunks
.map((chunk) => chunk.rule)
- .where((rule) => rule != null && rule is! HardSplitRule)
+ .where((rule) => rule != null)
.toSet()
.toList(growable: false),
blockIndentation = blockIndentation,
diff --git a/lib/src/line_splitting/rule_set.dart b/lib/src/line_splitting/rule_set.dart
index 37c5e83..714875d 100644
--- a/lib/src/line_splitting/rule_set.dart
+++ b/lib/src/line_splitting/rule_set.dart
@@ -23,10 +23,18 @@
RuleSet._(this._values);
/// Returns `true` of [rule] is bound in this set.
- bool contains(Rule rule) => _values[rule.index] != null;
+ bool contains(Rule rule) {
+ // Treat hardened rules as implicitly bound.
+ if (rule.isHardened) return true;
+
+ return _values[rule.index] != null;
+ }
/// Gets the bound value for [rule] or [Rule.unsplit] if it is not bound.
int getValue(Rule rule) {
+ // Hardened rules are implicitly bound.
+ if (rule.isHardened) return rule.fullySplitValue;
+
var value = _values[rule.index];
if (value != null) return value;
@@ -56,11 +64,20 @@
/// If an unbound rule gets constrained to `-1` (meaning it must split, but
/// can split any way it wants), invokes [onSplitRule] with it.
bool tryBind(List<Rule> rules, Rule rule, int value, onSplitRule(Rule rule)) {
+ assert(!rule.isHardened);
+
_values[rule.index] = value;
// Test this rule against the other rules being bound.
for (var other in rule.constrainedRules) {
- var otherValue = _values[other.index];
+ var otherValue;
+ // Hardened rules are implicitly bound.
+ if (other.isHardened) {
+ otherValue = other.fullySplitValue;
+ } else {
+ otherValue = _values[other.index];
+ }
+
var constraint = rule.constrain(value, other);
if (otherValue == null) {
diff --git a/lib/src/line_splitting/solve_state.dart b/lib/src/line_splitting/solve_state.dart
index 4b58b50..47489b6 100644
--- a/lib/src/line_splitting/solve_state.dart
+++ b/lib/src/line_splitting/solve_state.dart
@@ -113,11 +113,7 @@
/// Gets the value to use for [rule], either the bound value or
/// [Rule.unsplit] if it isn't bound.
- int getValue(Rule rule) {
- if (rule is HardSplitRule) return Rule.unsplit;
-
- return _ruleValues.getValue(rule);
- }
+ int getValue(Rule rule) => _ruleValues.getValue(rule);
/// Returns `true` if this state is a better solution to use as the final
/// result than [other].
@@ -437,7 +433,6 @@
/// live rules were added.
bool _addLiveRules(Rule rule) {
if (rule == null) return false;
- if (rule is HardSplitRule) return false;
var added = false;
for (var constrained in rule.allConstrainedRules) {
@@ -472,7 +467,7 @@
}
var rule = _splitter.chunks[i].rule;
- if (rule != null && rule is! HardSplitRule) {
+ if (rule != null) {
if (_ruleValues.contains(rule)) {
boundInLine.add(rule);
} else {
diff --git a/lib/src/line_writer.dart b/lib/src/line_writer.dart
index 69f8c27..9fb5d39 100644
--- a/lib/src/line_writer.dart
+++ b/lib/src/line_writer.dart
@@ -144,7 +144,7 @@
if (debug.traceLineWriter) {
debug.log(debug.green("\nWriting:"));
- debug.dumpChunks(start, chunks);
+ debug.dumpChunks(0, chunks);
debug.log();
}
diff --git a/lib/src/rule/argument.dart b/lib/src/rule/argument.dart
index f991232..046ff10 100644
--- a/lib/src/rule/argument.dart
+++ b/lib/src/rule/argument.dart
@@ -124,8 +124,6 @@
splitsOnInnerRules =
splitsOnInnerRules != null ? splitsOnInnerRules : false;
- bool isSplit(int value, Chunk chunk) => value != Rule.unsplit;
-
int constrain(int value, Rule other) {
var constrained = super.constrain(value, other);
if (constrained != null) return constrained;
@@ -194,10 +192,7 @@
String toString() => "*Pos${super.toString()}";
- bool isSplit(int value, Chunk chunk) {
- // Don't split at all.
- if (value == Rule.unsplit) return false;
-
+ bool isSplitAtValue(int value, Chunk chunk) {
// Split only before the first argument. Keep the entire argument list
// together on the next line.
if (value == 1) return chunk == _arguments.first;
@@ -298,17 +293,12 @@
_first = chunk;
}
- bool isSplit(int value, Chunk chunk) {
- switch (value) {
- case Rule.unsplit:
- return false;
- case 1:
- return chunk == _first;
- case 2:
- return true;
- }
+ bool isSplitAtValue(int value, Chunk chunk) {
+ // Only split before the first argument.
+ if (value == 1) return chunk == _first;
- throw "unreachable";
+ // Split before every argument.
+ return true;
}
String toString() => "Named${super.toString()}";
diff --git a/lib/src/rule/combinator.dart b/lib/src/rule/combinator.dart
index 0bcd37e..1d73031 100644
--- a/lib/src/rule/combinator.dart
+++ b/lib/src/rule/combinator.dart
@@ -80,12 +80,8 @@
_names.last.add(chunk);
}
- bool isSplit(int value, Chunk chunk) {
+ bool isSplitAtValue(int value, Chunk chunk) {
switch (value) {
- case Rule.unsplit:
- // Don't split at all.
- return false;
-
case 1:
// Just split at the combinators.
return _combinators.contains(chunk);
@@ -106,11 +102,9 @@
// Split everything.
return true;
- case 4:
+ default:
return true;
}
-
- throw "unreachable";
}
/// Returns `true` if [chunk] is for a combinator or a name in the
diff --git a/lib/src/rule/metadata.dart b/lib/src/rule/metadata.dart
index 3964a65..9abbe8f 100644
--- a/lib/src/rule/metadata.dart
+++ b/lib/src/rule/metadata.dart
@@ -16,7 +16,7 @@
///
/// Also, if the annotations split, we force the entire parameter list to fully
/// split, both named and positional.
-class MetadataRule extends SimpleRule {
+class MetadataRule extends Rule {
Rule _positionalRule;
Rule _namedRule;
diff --git a/lib/src/rule/rule.dart b/lib/src/rule/rule.dart
index 8eedb4d..e01d3b0 100644
--- a/lib/src/rule/rule.dart
+++ b/lib/src/rule/rule.dart
@@ -9,7 +9,7 @@
/// A constraint that determines the different ways a related set of chunks may
/// be split.
-abstract class Rule extends FastHash {
+class Rule extends FastHash {
/// Rule value that splits no chunks.
///
/// Every rule is required to treat this value as fully unsplit.
@@ -26,19 +26,26 @@
/// which aren't. Values range from zero to one minus this. Value zero
/// always means "no chunks are split" and increasing values by convention
/// mean increasingly undesirable splits.
- int get numValues;
+ ///
+ /// By default, a rule has two values: fully unsplit and fully split.
+ int get numValues => 2;
/// The rule value that forces this rule into its maximally split state.
///
/// By convention, this is the highest of the range of allowed values.
int get fullySplitValue => numValues - 1;
- int get cost => Cost.normal;
+ final int cost;
/// During line splitting [LineSplitter] sets this to the index of this
/// rule in its list of rules.
int index;
+ /// If `true`, the rule has been "hardened" meaning it's been placed into a
+ /// permanent "must fully split" state.
+ bool get isHardened => _isHardened;
+ bool _isHardened = false;
+
/// The other [Rule]s that "surround" this one (and care about that fact).
///
/// In many cases, if a split occurs inside an expression, surrounding rules
@@ -69,7 +76,38 @@
/// rules.
bool get splitsOnInnerRules => true;
- bool isSplit(int value, Chunk chunk);
+ Rule([int cost]) : cost = cost ?? Cost.normal;
+
+ /// Creates a new rule that is already fully split.
+ Rule.hard() : cost = 0 {
+ // Set the cost to zero since it will always be applied, so there's no
+ // point in penalizing it.
+ //
+ // Also, this avoids doubled counting in literal blocks where there is both
+ // a split in the outer chunk containing the block and the inner hard split
+ // between the elements or statements.
+ harden();
+ }
+
+ /// Fixes this rule into a "fully split" state.
+ void harden() {
+ _isHardened = true;
+ }
+
+ /// Returns `true` if [chunk] should split when this rule has [value].
+ bool isSplit(int value, Chunk chunk) {
+ if (_isHardened) return true;
+
+ if (value == Rule.unsplit) return false;
+
+ // Let the subclass decide.
+ return isSplitAtValue(value, chunk);
+ }
+
+ /// Subclasses can override this to determine which values split which chunks.
+ ///
+ /// By default, this assumes every chunk splits.
+ bool isSplitAtValue(value, chunk) => true;
/// Given that this rule has [value], determine if [other]'s value should be
/// constrained.
@@ -146,36 +184,3 @@
String toString() => "$id";
}
-
-/// A rule that always splits a chunk.
-class HardSplitRule extends Rule {
- int get numValues => 1;
-
- /// It's always going to be applied, so there's no point in penalizing it.
- ///
- /// Also, this avoids doubled counting in literal blocks where there is both
- /// a split in the outer chunk containing the block and the inner hard split
- /// between the elements or statements.
- int get cost => 0;
-
- /// It's always split anyway.
- bool get splitsOnInnerRules => false;
-
- bool isSplit(int value, Chunk chunk) => true;
-
- String toString() => "Hard";
-}
-
-/// A basic rule that has two states: unsplit or split.
-class SimpleRule extends Rule {
- /// Two values: 0 is unsplit, 1 is split.
- int get numValues => 2;
-
- final int cost;
-
- SimpleRule([int cost]) : cost = cost != null ? cost : Cost.normal;
-
- bool isSplit(int value, Chunk chunk) => value != Rule.unsplit;
-
- String toString() => "Simple${super.toString()}";
-}
diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart
index 63e6f23..79d3e86 100644
--- a/lib/src/source_visitor.dart
+++ b/lib/src/source_visitor.dart
@@ -307,7 +307,7 @@
visitNodes(node.cascadeSections, between: zeroSplit);
builder.endRule();
} else {
- builder.startRule(new HardSplitRule());
+ builder.startRule(new Rule.hard());
zeroSplit();
visitNodes(node.cascadeSections, between: zeroSplit);
builder.endRule();
@@ -1727,7 +1727,7 @@
builder.nestExpression();
// This rule is ended by visitExpressionFunctionBody().
- builder.startLazyRule(new SimpleRule(Cost.arrow));
+ builder.startLazyRule(new Rule(Cost.arrow));
}
if (parameters != null) {
@@ -1828,7 +1828,7 @@
// Always use a hard rule to split the elements. The parent chunk of
// the collection will handle the unsplit case, so this only comes
// into play when the collection is split.
- var rule = new HardSplitRule();
+ var rule = new Rule.hard();
builder.startRule(rule);
// If a collection contains a line comment, we assume it's a big complex
@@ -2118,7 +2118,7 @@
/// Writes a single space split with its own rule.
void soloSplit([int cost]) {
- builder.startRule(new SimpleRule(cost));
+ builder.startRule(new Rule(cost));
split();
builder.endRule();
}
diff --git a/test/regression/0200/0221.unit b/test/regression/0200/0221.unit
index 74cf12c..ab5fb23 100644
--- a/test/regression/0200/0221.unit
+++ b/test/regression/0200/0221.unit
@@ -35,11 +35,13 @@
void _updateChart() {
if (_model.settings != null) {
_chart.update((ChartSettings.builder()
- ..ids.addAll(_model.ids)
- ..statusFilter = StatusFilter.ALL
- ..dateRange = chartDates.toChartDateRange(_model.settings.dateRange.value)
- ..segmentationDimension = _model.segmentation
- ..context = ChartContext.empty).build());
+ ..ids.addAll(_model.ids)
+ ..statusFilter = StatusFilter.ALL
+ ..dateRange =
+ chartDates.toChartDateRange(_model.settings.dateRange.value)
+ ..segmentationDimension = _model.segmentation
+ ..context = ChartContext.empty)
+ .build());
}
}
>>> (indent 2)
