Remove a bunch of TODO(tall) and TODO(perf) comments that aren't needed. #1484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These are all things we have either done already, have filed issues, or I don't have any plans to do at this point.
munificent
commented
May 13, 2024
| // example I tried. Leaving this here as a TODO to investigate more when | ||
| // there are other benchmarks we can try. | ||
| var solutions = <Solution>[]; | ||
|
|
There was a problem hiding this comment.
I tried this optimization, but it had no measurable effects on any of the benchmarks or on formatting the entire Flutter repo.
| @@ -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? | |||
There was a problem hiding this comment.
This is done now as of e49a9f3.
| @@ -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. | |||
There was a problem hiding this comment.
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?)
| // 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. |
| @@ -47,7 +47,6 @@ someReceiverObject.property1.property2 | |||
| .property5 | |||
| .property6; | |||
| <<< | |||
| ### TODO(tall): Allow splitting between successive indexes. | |||
There was a problem hiding this comment.
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.
natebosch
approved these changes
May 13, 2024
kallentu
approved these changes
May 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are all things we have either done already, have filed issues, or I don't have any plans to do at this point.