DRILL-1942-hygiene #120
DRILL-1942-hygiene #120
Conversation
- Formatting - @OVERRIDES - finals - some AutoCloseable additions - new isCancelled() abstract method on FragmentManager, implemented on subclasses Added missing new abstract method isCancelled()
cwestin
force-pushed
the
hygiene-alpha
branch
from
993844f
to
bdd0428
Compare
| @@ -211,8 +220,11 @@ public TransferPair makeTransferPair(ValueVector to) { | |||
|
|
|||
| public void transferTo(BitVector target) { | |||
| target.clear(); | |||
| if (target.data != null) { | |||
| target.data.release(); | |||
There was a problem hiding this comment.
release(1) ?
Also, do you intend to change from retain() to retain(1) across the source codes? Seems some places are still using retain(). Do a quick search will find couple of places, including BitVector.load(), VariableLengthVectors.java:load() etc.
There was a problem hiding this comment.
Originally I did change all the release()s and retain()s to (1) in all files. But Jacques made me undo them in a few files he reviewed in another patch, and I haven't searched to see if there are new additions since I made the original changes. I think it's worth saving the function call for some low-level things (such as value vectors). What would you prefer?
There was a problem hiding this comment.
I think the current form is good. I'll merge the patch as it is.
|
+1 LGTM. |
| * | ||
| * @return true if the FragmentManager has been cancelled. | ||
| */ | ||
| boolean isCancelled(); |
There was a problem hiding this comment.
I don't see this method being used anywhere. Is this change intended?
There was a problem hiding this comment.
"Added missing new abstract method isCancelled(); needed for upcoming patches"
There was a problem hiding this comment.
My bad.. I did not see that mentioned in the commit message, and I did not realize the PR was made with comments in addition to the commit message.
There was a problem hiding this comment.
Yes, this is a prerequisite for another patch that is coming.
Added missing new abstract method isCancelled(); needed for upcoming patches
Unit tests passed
Precommit suite passed (except for Windows)