Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714503003:
Allow passing in strings of length zero to FileWrapper::Write without closing the file. (Closed)
|
Created:
4 years, 10 months ago by terelius Modified:
4 years, 8 months ago Reviewers:
Henrik Grunell WebRTC CC:
webrtc-reviews_webrtc.org, henrika_webrtc, zhengzhonghou_agora.io, tterriberry_mozilla.com, fengyue_agora.io, peah-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow passing in strings of length zero to FileWrapper::Write without closing the file.
BUG=webrtc:5253
Committed: https://crrev.com/6b1968ea70fc69da04aa49b0a5305cda51d66d38
Cr-Commit-Position: refs/heads/master@{#12132}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Don't close the file #Patch Set 3 : Close the file upon error to be consistent with other functions #
Total comments: 2
Patch Set 4 : Nit #Messages
Total messages: 24 (8 generated)
terelius@webrtc.org changed reviewers: + henrikg@webrtc.org
Please take a look.
https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... File webrtc/system_wrappers/source/file_impl.cc (right): https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:246: size_in_bytes_ += num_bytes; Keep this inside the if block below. Not needed if we close the file. https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:247: if (num_bytes == length) { The original code is unclear. Good to change this. 1. Checking that we actually wrote the amount we wanted is good. 2. Should we really allow |length| == 0? The user of this class shouldn't really call if there's nothing to write. 3. It seems strange that the file is closed here at all. The user should decide what to do if an error happens. I don't want to block this fix. If (2) and (3) this causes larger changes outside this class, I'm fine with not changing that. Add a todo to clean this up. https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:251: CloseFileImpl(); Actually, do this if above condition isn't met instead. And let the successful return be last.
https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... File webrtc/system_wrappers/source/file_impl.cc (right): https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:247: if (num_bytes == length) { On 2016/02/19 15:04:38, Henrik Grunell (webrtc) wrote: > The original code is unclear. Good to change this. > > 1. Checking that we actually wrote the amount we wanted is good. > > 2. Should we really allow |length| == 0? The user of this class shouldn't really > call if there's nothing to write. > > 3. It seems strange that the file is closed here at all. The user should decide > what to do if an error happens. > > I don't want to block this fix. If (2) and (3) this causes larger changes > outside this class, I'm fine with not changing that. Add a todo to clean this > up. I agree on 1 and 3. I'd also prefer not closing the file. If you think that's ok, I could shorten the function by 5 lines. I guess that the original author chose to close the file because the file position indicator is undetermined after an fwrite error. Regarding 2: Although it is pointless to write an empty string, I think it is nevertheless useful to allow since the string could be the result of some computation. If Write disallows empty strings you'd have to add a check at every call site (unless you can guarantee that the computation can't produce empty strings). Moreover, those checks would duplicate checks that are done by fwrite anyway. Secondly, the description of the return value becomes more clear if empty strings are allowed. Compare "true if |length| bytes were written" to "true if |length| bytes were written and |length| is non-zero".
On 2016/02/19 16:05:52, terelius wrote: > https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... > File webrtc/system_wrappers/source/file_impl.cc (right): > > https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... > webrtc/system_wrappers/source/file_impl.cc:247: if (num_bytes == length) { > On 2016/02/19 15:04:38, Henrik Grunell (webrtc) wrote: > > The original code is unclear. Good to change this. > > > > 1. Checking that we actually wrote the amount we wanted is good. > > > > 2. Should we really allow |length| == 0? The user of this class shouldn't > really > > call if there's nothing to write. > > > > 3. It seems strange that the file is closed here at all. The user should > decide > > what to do if an error happens. > > > > I don't want to block this fix. If (2) and (3) this causes larger changes > > outside this class, I'm fine with not changing that. Add a todo to clean this > > up. > > I agree on 1 and 3. I'd also prefer not closing the file. If you think that's > ok, I could shorten the function by 5 lines. I guess that the original author > chose to close the file because the file position indicator is undetermined > after an fwrite error. > > Regarding 2: Although it is pointless to write an empty string, I think it is > nevertheless useful to allow since the string could be the result of some > computation. If Write disallows empty strings you'd have to add a check at every > call site (unless you can guarantee that the computation can't produce empty > strings). Moreover, those checks would duplicate checks that are done by fwrite > anyway. > > Secondly, the description of the return value becomes more clear if empty > strings are allowed. Compare "true if |length| bytes were written" to "true if > |length| bytes were written and |length| is non-zero". I'm fine with keeping (2) as it is for your mentioned reasons. For (3) you can choose what you prefer.
https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... File webrtc/system_wrappers/source/file_impl.cc (right): https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:246: size_in_bytes_ += num_bytes; On 2016/02/19 15:04:38, Henrik Grunell (webrtc) wrote: > Keep this inside the if block below. Not needed if we close the file. By not closing the file, we can remove the entire if block. https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:251: CloseFileImpl(); On 2016/02/19 15:04:38, Henrik Grunell (webrtc) wrote: > Actually, do this if above condition isn't met instead. And let the successful > return be last. By not closing the file, we can instead return the result of the condition without branching.
lgtm https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... File webrtc/system_wrappers/source/file_impl.cc (right): https://codereview.webrtc.org/1714503003/diff/1/webrtc/system_wrappers/source... webrtc/system_wrappers/source/file_impl.cc:251: CloseFileImpl(); On 2016/02/19 16:50:29, terelius wrote: > On 2016/02/19 15:04:38, Henrik Grunell (webrtc) wrote: > > Actually, do this if above condition isn't met instead. And let the successful > > return be last. > > By not closing the file, we can instead return the result of the condition > without branching. Does the file need to be closed elsewhere instead? Actually, we don't know the reason for failure, so we really can't. Just make sure removing the close here doesn't cause any trouble.
I changed back so that Write still closes the file if a write error occurs. This keeps the behavior consistent with the other functions (WriteText and Read). Can you confirm that I still have approval to land it?
lgtm, nit is optional. https://codereview.webrtc.org/1714503003/diff/40001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/file_impl.cc (left): https://codereview.webrtc.org/1714503003/diff/40001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/file_impl.cc:244: Nit: I think this empty line is nice to keep for readability.
https://codereview.webrtc.org/1714503003/diff/40001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/file_impl.cc (left): https://codereview.webrtc.org/1714503003/diff/40001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/file_impl.cc:244: On 2016/03/24 10:34:11, Henrik Grunell (webrtc) wrote: > Nit: I think this empty line is nice to keep for readability. I agree. Done.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrikg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1714503003/#ps60001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714503003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714503003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714503003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714503003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714503003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714503003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow passing in strings of length zero to FileWrapper::Write without closing the file. BUG=webrtc:5253 ========== to ========== Allow passing in strings of length zero to FileWrapper::Write without closing the file. BUG=webrtc:5253 Committed: https://crrev.com/6b1968ea70fc69da04aa49b0a5305cda51d66d38 Cr-Commit-Position: refs/heads/master@{#12132} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6b1968ea70fc69da04aa49b0a5305cda51d66d38 Cr-Commit-Position: refs/heads/master@{#12132} |
