[IO-634] Make getCause synchronized and use a Deque instead of a Stack#64
Conversation
kinow
left a comment
There was a problem hiding this comment.
Looks good to me! Not merging right now as I haven't figured out why Travis is so broken.
| */ | ||
| @Override | ||
| public IOException getCause() { | ||
| public synchronized IOException getCause() { |
There was a problem hiding this comment.
Interesting! Didn't know Throwable's getCause was synchronized - at least in OpenJDK https://github.com/openjdk/jdk/blob/843fe9eb03ef33692edba925666e0214ae6934e6/src/java.base/share/classes/java/lang/Throwable.java#L421
|
Ping. |
| int textIdx = 0; | ||
| int wcsIdx = 0; | ||
| final Stack<int[]> backtrack = new Stack<>(); | ||
| final Deque<int[]> backtrack = new ArrayDeque<>(wcs.length); |
There was a problem hiding this comment.
Looks reasonable to me. Stack is synchronized I think, while Deque is not. But backtrack is used in this method without any locks, so Stack doesn't appear to be used specifically for that.
|
@wilx could you rebase your code on |
|
Thanks @wilx ! Build passing on Travis. @garydgregory I haven't looked at commons-io in a while. Would you be able to have another look at this PR and double check if it looks OK, and whether we'd need a JIRA for this or not, please? (no worries if busy, I can drop an e-mail to dev-list later this weekend) |
|
Yes, we should have a JIRA and this PR needs a description and a title that reflects the changes. The current title "Two small tweaks." is meaningless. |
|
Thanks @garydgregory . @wilx could you create a JIRA and rename the PR, please? |
|
I will but TBH, reluctantly. |
|
JIRA issue here: https://issues.apache.org/jira/browse/IO-634 |
|
That's just to have the issue in the changelog (when merging we'll add it to The title is still vague. What if the PR/issue have a title like "Make getCause synchronized and use a Deque instead of Stack" ? Or something along that line. |
No description provided.