From 222f670f40a1b3f10e15df936b25793454d9b900 Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Mon, 13 May 2024 14:30:25 -0700 Subject: [PATCH] Remove a bunch of TODO(tall) and TODO(perf) comments that aren't needed. (#1484) These are all things we have either done already, have filed issues, or I don't have any plans to do at this point. --- lib/src/back_end/code_writer.dart | 12 ++++-------- lib/src/back_end/solution.dart | 18 +----------------- lib/src/front_end/ast_node_visitor.dart | 3 --- lib/src/front_end/piece_writer.dart | 2 -- lib/src/piece/assign.dart | 12 ------------ test/tall/invocation/chain_postfix.stmt | 1 - 6 files changed, 5 insertions(+), 43 deletions(-) diff --git a/lib/src/back_end/code_writer.dart b/lib/src/back_end/code_writer.dart index afd15950..7f81f413 100644 --- a/lib/src/back_end/code_writer.dart +++ b/lib/src/back_end/code_writer.dart @@ -118,15 +118,11 @@ class CodeWriter { /// /// If [text] contains any internal newlines, the caller is responsible for /// also calling [handleNewline()]. + /// + /// When possible, avoid calling this directly. Instead, any input code + /// lexemes should be written to TextPieces which then call this. That way, + /// selections inside lexemes are correctly updated. void write(String text) { - // TODO(tall): Calling this directly from pieces outside of TextPiece may - // not handle selections as gracefully as we could. A selection marker may - // get pushed past the text written here. Currently, this is only called - // directly for commas in list-like things, and `;` in for loops. In - // general, it's better for all text written to the output to live inside - // TextPieces because that will preserve selection markers. Consider doing - // something smarter for commas in lists and semicolons in for loops. - _flushWhitespace(); _buffer.write(text); _column += text.length; diff --git a/lib/src/back_end/solution.dart b/lib/src/back_end/solution.dart index 8d5c627d..8f2d3add 100644 --- a/lib/src/back_end/solution.dart +++ b/lib/src/back_end/solution.dart @@ -233,27 +233,11 @@ class Solution implements Comparable { // end (or a winner). if (expandPiece == null) return const []; - // TODO(perf): If `_invalidPiece == expandPiece`, then we know that the - // first state leads to an invalid solution, so there's no point in trying - // to expand to a solution that binds `expandPiece` to - // `expandPiece.states[0]`. We should be able to do: - // - // Iterable states = expandPiece.states; - // if (_invalidPiece == expandPiece) { - // print('skip $expandPiece ${states.first}'); - // states = states.skip(1); - // } - // - // And then use `states` below. But when I tried that, it didn't seem to - // make any noticeable performance difference on the one pathological - // example I tried. Leaving this here as a TODO to investigate more when - // there are other benchmarks we can try. - var solutions = []; - // For each state that the expanding piece can be in, create a new solution // that inherits all of the bindings of this one, and binds the expanding // piece to that state (along with any further pieces constrained by that // one). + var solutions = []; for (var state in expandPiece.states) { var newStates = {..._pieceStates}; diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 33919f9e..8dc1f320 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -523,9 +523,6 @@ class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { if (node.members.isEmpty) { // If there are no members, format the constants like a delimited list. // This keeps the enum declaration on one line if it fits. - // TODO(tall): The old style preserves blank lines and newlines between - // enum values. A newline will also force the enum to split even if it - // would otherwise fit. Do we want to do that with the new style too? var builder = DelimitedListBuilder(this, const ListStyle(spaceWhenUnsplit: true)); diff --git a/lib/src/front_end/piece_writer.dart b/lib/src/front_end/piece_writer.dart index 4609d91c..a70f3e72 100644 --- a/lib/src/front_end/piece_writer.dart +++ b/lib/src/front_end/piece_writer.dart @@ -302,8 +302,6 @@ class PieceWriter { void _flushSpace() { if (!_pendingSpace) return; - // TODO(perf): See if we can make SpacePiece a constant to avoid creating - // multiple. _pieces.last.add(SpacePiece()); _pendingSpace = false; } diff --git a/lib/src/piece/assign.dart b/lib/src/piece/assign.dart index 52bb8dad..973c6095 100644 --- a/lib/src/piece/assign.dart +++ b/lib/src/piece/assign.dart @@ -121,18 +121,6 @@ class AssignPiece extends Piece { _canBlockSplitLeft = canBlockSplitLeft, _canBlockSplitRight = canBlockSplitRight; - // TODO(tall): The old formatter allows the first operand of a split - // conditional expression to be on the same line as the `=`, as in: - // - // var value = condition - // ? thenValue - // : elseValue; - // - // For now, we do not implement this special case behavior. Once more of the - // language is implemented in the new back end and we can run the formatter - // on a large corpus of code, we can try it out and see if the special case - // behavior is worth it. - @override List get additionalStates => [ // If at least one operand can block split, allow splitting in operands diff --git a/test/tall/invocation/chain_postfix.stmt b/test/tall/invocation/chain_postfix.stmt index 80b11b3e..9df1e9c8 100644 --- a/test/tall/invocation/chain_postfix.stmt +++ b/test/tall/invocation/chain_postfix.stmt @@ -47,7 +47,6 @@ someReceiverObject.property1.property2 .property5 .property6; <<< -### TODO(tall): Allow splitting between successive indexes. someReceiverObject .property1 .property2