Use more records.#5781
Merged
Merged
Conversation
I noticed an IntelliJ suggestion for this in one file in unknown commit, so I decided to turn it loose. Notes: - If anything gets us in trouble here, I would expect it to be a performance regression or behavioral bug from suddenly having a value-based `equals` implementation. - Out of fear of that, I reverted a proposed change to `ResultingStore` in `AbstractNullnessPropagationTransfer`. But I don't have confidence that (a) that would have been a problem or (b) that nothing else in this CL will be a problem. - Gemini notices that, in `RefactoringCollection`, `DelegatingDescriptionListener` does end up in a `HashMultimap`, but it seems unlikely that we care about equality there. (Perhaps we should be using a `ListMultimap` there, anyway, including to preserve ordering (though we could also accomplish that with `LinkedHashMultimap` if we wanted to keep to `SetMultimap`). - IntelliJ was even ready to update documentation (`Immutable.md` and `ImmutableEnumChecker.md`), but I reverted that.) - I reverted the change to `CompilerBasedAbstractTest`, since that would created a record with an array member, upsetting [ArrayRecordComponent](http://errorprone.info/bugpattern/ArrayRecordComponent), which wasn't completely trivial to fix. - I wonder if any sort of named class has been overkill in some cases here. Like, maybe `RefasterSuppressionHelper` would be just as well off with a static method that returns an instance of anonymous type? Maybe some of the matchers could be implemented with lambdas? I didn't get into that. - I'm still not entirely sure what to think of using `record` for classes that aren't what I think of as "value types," like `DelegatingDescriptionListener` in `RefactoringCollection` or the aforementioned matchers. Since most of the types here are implementation details, I don't worry about them as much as I worried in b/380275991, but I'm still personally not sure what to think. PiperOrigin-RevId: 912209326
copybara-service
Bot
force-pushed
the
test_912116354
branch
from
4ef7f5a to
6753f23
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Use more records.
I noticed an IntelliJ suggestion for this in one file in unknown commit, so I decided to turn it loose.
Notes:
equalsimplementation.ResultingStoreinAbstractNullnessPropagationTransfer. But I don't have confidence that (a) that would have been a problem or (b) that nothing else in this CL will be a problem.RefactoringCollection,DelegatingDescriptionListenerdoes end up in aHashMultimap, but it seems unlikely that we care about equality there. (Perhaps we should be using aListMultimapthere, anyway, including to preserve ordering (though we could also accomplish that withLinkedHashMultimapif we wanted to keep toSetMultimap).Immutable.mdandImmutableEnumChecker.md), but I reverted that.)CompilerBasedAbstractTest, since that would created a record with an array member, upsetting ArrayRecordComponent, which wasn't completely trivial to fix.RefasterSuppressionHelperwould be just as well off with a static method that returns an instance of anonymous type? Maybe some of the matchers could be implemented with lambdas? I didn't get into that.recordfor classes that aren't what I think of as "value types," likeDelegatingDescriptionListenerinRefactoringCollectionor the aforementioned matchers. Since most of the types here are implementation details, I don't worry about them as much as I worried in b/380275991, but I'm still personally not sure what to think.