Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove a bunch of TODO(tall) and TODO(perf) comments that aren't needed. #1484

Merged
merged 1 commit into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>[];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this optimization, but it had no measurable effects on any of the benchmarks or on formatting the entire Flutter repo.

// 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 @@ -520,9 +520,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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done now as of e49a9f3.

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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this optimization a while back and it actually made things slower. (I'm guessing because it required SpacePiece to implement Piece instead of extending it and that in turn made a bunch of calls on Piece methods polymorphic?)

_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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked by #1465.


@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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this just isn't worth doing. It never looks good and users are better off either reorganizing their code or allowing it to run long.

someReceiverObject
.property1
.property2
Expand Down