Skip to content

Commit

Permalink
Remove a bunch of TODO(tall) and TODO(perf) comments that aren't need…
Browse files Browse the repository at this point in the history
…ed. (#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.
  • Loading branch information
munificent authored May 13, 2024
1 parent e49a9f3 commit 222f670
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 43 deletions.
12 changes: 4 additions & 8 deletions lib/src/back_end/code_writer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
18 changes: 1 addition & 17 deletions lib/src/back_end/solution.dart
Original file line number Diff line number Diff line change
Expand Up @@ -233,27 +233,11 @@ class Solution implements Comparable<Solution> {
// 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<State> 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 = <Solution>[];

// 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 = <Solution>[];
for (var state in expandPiece.states) {
var newStates = {..._pieceStates};

Expand Down
3 changes: 0 additions & 3 deletions lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -523,9 +523,6 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> 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));

Expand Down
2 changes: 0 additions & 2 deletions lib/src/front_end/piece_writer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
12 changes: 0 additions & 12 deletions lib/src/piece/assign.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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<State> get additionalStates => [
// If at least one operand can block split, allow splitting in operands
Expand Down
1 change: 0 additions & 1 deletion test/tall/invocation/chain_postfix.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ someReceiverObject.property1.property2
.property5
.property6;
<<<
### TODO(tall): Allow splitting between successive indexes.
someReceiverObject
.property1
.property2
Expand Down

0 comments on commit 222f670

Please sign in to comment.