ticket:11463: ICU4J ought to always close() resources

ticket:11463: ICU4J ought to always close() resources

R=yoshito_umaoka@us.ibm.com

Committed: http://bugs.icu-project.org/trac/changeset/37034

[markus.icu, 2015-02-05 01:50:27]

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUBinary.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:360: ByteBuffer buffer =
null;
Other than the added @SuppressWarnings, how is this change helping close the
stream?

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:399: channel.close();
redundant, and the comment was supposed to indicate it: FIS.close() documents
"If this stream has an associated channel then the channel is closed as well."

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java:340:
@SuppressWarnings("resource")  // Closed by getByteBufferFromInputStream().
If it helps, we could rename that to 
getByteBufferFromInputStreamAndCloseStream() ...

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/URLHandler.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/URLHandler.java:47: try {
Three nested "try"... Optional: Reduce nesting by moving variable declarations
out (here: br) and moving the finally block to the outer, existing "try", with a
conditional close? (if (br != null) { br.close(); })

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java:244: try {
I would move this up to just before the getStream() call; logically, we want to
close out the old stuff before opening the new one.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java:258:
reader.close();
Add reader = null;  ?

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java:131:
throw new MissingResourceException(
Probably better ICUUncheckedIOException

https://codereview.appspot.com/199390043/diff/20001/main/tests/collate/src/co...
File main/tests/collate/src/com/ibm/icu/dev/test/collator/CollationThaiTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/collate/src/co...
main/tests/collate/src/com/ibm/icu/dev/test/collator/CollationThaiTest.java:186:
if (in != null) {
This loses some error output. I suggest
    if (in == null) {
        errln("Error: could not open ... (copy from the trunk version) ...
return;
    } else {
        try in.close() ...

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:30:
BufferedReader bidiTestFile = TestUtil.getDataReader("unicode/BidiTest.txt");
Is it worth changing test code? Maybe just @SuppressWarnings("resource")  //
test, don't care

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:38:
outerLoop: while (errorCount < 10 && (line = bidiTestFile.readLine()) != null) {
Please retain the label at the start of a separate line.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:171:
outerLoop: while (errorCount < 20 && (line = bidiTestFile.readLine()) != null) {
see above

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1712:
@SuppressWarnings("resource")  // Closed by ResourceReader.
Confusing. Move to just before the declaration of is?

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1804:
break;
Doesn't the finally block get executed even when you return here?

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1885:
break;
ditto

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/lang/UCharacterTest.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/lang/UCharacterTest.java:812: break;
ok to keep the original return? see previous file's comment.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/rbbi/RBBITestExtended.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/rbbi/RBBITestExtended.java:90:
is.close();
Why not just isr.close() here which should in turn to is.close() too?

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java:123:
inputStream.close();
Please add this finally block to the inner "try"; I don't think we need to add
the outer level.

Should we set inputStream to null after closing?

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java:218:
element_loop: while (entries.hasMoreElements()) {
Separate line for the label.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File
main/tests/core/src/com/ibm/icu/dev/test/stringprep/IDNAConformanceTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/stringprep/IDNAConformanceTest.java:301:
} finally {
Move this to the outer "try" and don't add the inner one?

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/text/SpoofCheckerTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/text/SpoofCheckerTest.java:102: }
finally {
I would move both finally blocks to the outer "try" and not add the inner
"try"s.

[roubert (google), 2015-02-13 15:31:27]

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUBinary.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:360: ByteBuffer buffer =
null;
On 2015/02/05 01:50:26, markus.icu wrote:
> Other than the added @SuppressWarnings, how is this change helping close the
> stream?

The only purpose of this change is to make the Eclipse analyzer understand that
the resource really is being closed. (So that it doesn't show a false positive,
which would make it harder to find the real problems.)

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUBinary.java:399: channel.close();
On 2015/02/05 01:50:26, markus.icu wrote:
> redundant, and the comment was supposed to indicate it: FIS.close() documents
> "If this stream has an associated channel then the channel is closed as well."

Done.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java:340:
@SuppressWarnings("resource")  // Closed by getByteBufferFromInputStream().
On 2015/02/05 01:50:26, markus.icu wrote:
> If it helps, we could rename that to 
> getByteBufferFromInputStreamAndCloseStream() ...

That's a very long name. But I've made the change now to see how it looks. Do
you think it's an improvement like this, or should I revert that change again?

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/URLHandler.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/URLHandler.java:47: try {
On 2015/02/05 01:50:26, markus.icu wrote:
> Three nested "try"... Optional: Reduce nesting by moving variable declarations
> out (here: br) and moving the finally block to the outer, existing "try", with
a
> conditional close? (if (br != null) { br.close(); })

That seems like too many changes at the same time, I'd rather do that
separately.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java:244: try {
On 2015/02/05 01:50:26, markus.icu wrote:
> I would move this up to just before the getStream() call; logically, we want
to
> close out the old stuff before opening the new one.

Done.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/data/ResourceReader.java:258:
reader.close();
On 2015/02/05 01:50:26, markus.icu wrote:
> Add reader = null;  ?

Done.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java:131:
throw new MissingResourceException(
On 2015/02/05 01:50:26, markus.icu wrote:
> Probably better ICUUncheckedIOException

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/collate/src/co...
File main/tests/collate/src/com/ibm/icu/dev/test/collator/CollationThaiTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/collate/src/co...
main/tests/collate/src/com/ibm/icu/dev/test/collator/CollationThaiTest.java:186:
if (in != null) {
On 2015/02/05 01:50:26, markus.icu wrote:
> This loses some error output. I suggest
>     if (in == null) {
>         errln("Error: could not open ... (copy from the trunk version) ...
> return;
>     } else {
>         try in.close() ...

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:30:
BufferedReader bidiTestFile = TestUtil.getDataReader("unicode/BidiTest.txt");
On 2015/02/05 01:50:26, markus.icu wrote:
> Is it worth changing test code? Maybe just @SuppressWarnings("resource")  //
> test, don't care

If the test code actually leaks resources, then the warning code in Android
which tracks all resources (and doesn't read @SuppressWarnings to suppress
logging for some of them) would fill up the test logs with warnings.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:38:
outerLoop: while (errorCount < 10 && (line = bidiTestFile.readLine()) != null) {
On 2015/02/05 01:50:26, markus.icu wrote:
> Please retain the label at the start of a separate line.

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/bidi/BiDiConformanceTest.java:171:
outerLoop: while (errorCount < 20 && (line = bidiTestFile.readLine()) != null) {
On 2015/02/05 01:50:26, markus.icu wrote:
> see above

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1712:
@SuppressWarnings("resource")  // Closed by ResourceReader.
On 2015/02/05 01:50:26, markus.icu wrote:
> Confusing. Move to just before the declaration of is?

I don't know why, but if I do that, then Eclipse doesn't understand the
annotation.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1804:
break;
On 2015/02/05 01:50:26, markus.icu wrote:
> Doesn't the finally block get executed even when you return here?

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1885:
break;
On 2015/02/05 01:50:26, markus.icu wrote:
> ditto

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/lang/UCharacterTest.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/lang/UCharacterTest.java:812: break;
On 2015/02/05 01:50:26, markus.icu wrote:
> ok to keep the original return? see previous file's comment.

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/rbbi/RBBITestExtended.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/rbbi/RBBITestExtended.java:90:
is.close();
On 2015/02/05 01:50:27, markus.icu wrote:
> Why not just isr.close() here which should in turn to is.close() too?

It is possible that new InputStreamReader() throws UnsupportedEncodingException
(according to the contract, not in practice as the "UTF-8" encoding always
should be supported), in which case there will be an InputStream object that
needs to be closed even though there is no InputStreamReader object.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java:123:
inputStream.close();
On 2015/02/05 01:50:27, markus.icu wrote:
> Please add this finally block to the inner "try";

That would leave two code paths through which close() is not going to be called,
when inDocMode() and when !params.stack.included, which in those cases actually
would leak this resource.

> Should we set inputStream to null after closing?

It won't hurt.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/serializable/CompatibilityTest.java:218:
element_loop: while (entries.hasMoreElements()) {
On 2015/02/05 01:50:27, markus.icu wrote:
> Separate line for the label.

Done.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File
main/tests/core/src/com/ibm/icu/dev/test/stringprep/IDNAConformanceTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/stringprep/IDNAConformanceTest.java:301:
} finally {
On 2015/02/05 01:50:27, markus.icu wrote:
> Move this to the outer "try" and don't add the inner one?

As the sequence of catch statements doesn't actually do anything useful, it's
possible to do the opposite instead and remove the outer try statement.

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/text/SpoofCheckerTest.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/text/SpoofCheckerTest.java:102: }
finally {
On 2015/02/05 01:50:27, markus.icu wrote:
> I would move both finally blocks to the outer "try" and not add the inner
> "try"s.

Such code would then need if-null statements instead. I think the nested code
really is nicer here, as it keeps all the logic tightly together on a few lines.

[markus.icu, 2015-02-13 17:14:50]

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java:340:
@SuppressWarnings("resource")  // Closed by getByteBufferFromInputStream().
On 2015/02/13 15:31:26, roubert (google) wrote:
> On 2015/02/05 01:50:26, markus.icu wrote:
> > If it helps, we could rename that to 
> > getByteBufferFromInputStreamAndCloseStream() ...
> 
> That's a very long name. But I've made the change now to see how it looks. Do
> you think it's an improvement like this, or should I revert that change again?

I think it helps, because it makes it clear that the caller need not worry about
the stream.

Optional: It might be ok now to remove the comments from the @SuppressWarnings,
but they are probably still somewhat useful, they make it clear what is being
suppressed.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/URLHandler.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/URLHandler.java:47: try {
On 2015/02/13 15:31:26, roubert (google) wrote:
> On 2015/02/05 01:50:26, markus.icu wrote:
> > Three nested "try"... Optional: Reduce nesting by moving variable
declarations
> > out (here: br) and moving the finally block to the outer, existing "try",
with
> a
> > conditional close? (if (br != null) { br.close(); })
> 
> That seems like too many changes at the same time, I'd rather do that
> separately.

I was suggesting these to reduce the number of changes from the trunk. It would
be nice if we didn't have to indent further than currently (also to keep "svn
blame" useful) unless not nesting makes the code noticeably harder to
understand.

https://codereview.appspot.com/199390043/diff/60001/main/classes/core/src/com...
File
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java
(right):

https://codereview.appspot.com/199390043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java:133:
"Failed to close() resource " + name);
Please add e as the second parameter (the cause).

https://codereview.appspot.com/199390043/diff/60001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java
(right):

https://codereview.appspot.com/199390043/diff/60001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1712:
@SuppressWarnings("resource")  // Closed by ResourceReader.
// InputStream is will be closed by the ResourceReader.

[roubert (google), 2015-02-13 18:17:10]

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java
(right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/ICUResourceBundleReader.java:340:
@SuppressWarnings("resource")  // Closed by getByteBufferFromInputStream().
On 2015/02/13 17:14:50, markus.icu wrote:
> On 2015/02/13 15:31:26, roubert (google) wrote:
> > On 2015/02/05 01:50:26, markus.icu wrote:
> > > If it helps, we could rename that to 
> > > getByteBufferFromInputStreamAndCloseStream() ...
> > 
> > That's a very long name. But I've made the change now to see how it looks.
Do
> > you think it's an improvement like this, or should I revert that change
again?
> 
> I think it helps, because it makes it clear that the caller need not worry
about
> the stream.
> 
> Optional: It might be ok now to remove the comments from the
@SuppressWarnings,
> but they are probably still somewhat useful, they make it clear what is being
> suppressed.

Acknowledged.

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
File main/classes/core/src/com/ibm/icu/impl/URLHandler.java (right):

https://codereview.appspot.com/199390043/diff/20001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/URLHandler.java:47: try {
On 2015/02/13 17:14:50, markus.icu wrote:
> I was suggesting these to reduce the number of changes from the trunk.

OK, I've now done a minimal diff change instead.

https://codereview.appspot.com/199390043/diff/60001/main/classes/core/src/com...
File
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java
(right):

https://codereview.appspot.com/199390043/diff/60001/main/classes/core/src/com...
main/classes/core/src/com/ibm/icu/impl/duration/impl/ResourceBasedPeriodFormatterDataService.java:133:
"Failed to close() resource " + name);
On 2015/02/13 17:14:50, markus.icu wrote:
> Please add e as the second parameter (the cause).

Done.

https://codereview.appspot.com/199390043/diff/60001/main/tests/core/src/com/i...
File main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java
(right):

https://codereview.appspot.com/199390043/diff/60001/main/tests/core/src/com/i...
main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java:1712:
@SuppressWarnings("resource")  // Closed by ResourceReader.
On 2015/02/13 17:14:50, markus.icu wrote:
> // InputStream is will be closed by the ResourceReader.

Done.

[markus.icu, 2015-02-13 19:38:14]

Looks good to me.

[roubert (google), 2015-02-13 20:16:31]

Committed patchset #6 (id:100001) manually as 37034 (presubmit successful).

[yoshito, 2015-02-13 20:59:49]

On 2015/02/13 20:16:31, roubert (google) wrote:
> Committed patchset #6 (id:100001) manually as 37034 (presubmit successful).

Thanks. I did not have bandwidth to review your changes in last two weeks..
Markus, thanks for the review.