Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1803833002:
Stop using some scoped_ptr features that unique_ptr doesn't have (Closed)
|
Created:
4 years, 9 months ago by kwiberg-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStop using some scoped_ptr features that unique_ptr doesn't have
No operator== that accepts one unique_ptr<T> and one T*. No implicit
conversion to bool. No rtc_make_scoped_ptr function.
BUG=webrtc:5520
Committed: https://crrev.com/1d50ee44fd53e228d40db2d6aa2f0300b736c550
Cr-Commit-Position: refs/heads/master@{#12048}
Patch Set 1 #
Total comments: 15
Patch Set 2 : To not not, or not to not not? That's not not the question. #Patch Set 3 : More review comments fixed #
Total comments: 1
Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 36 (16 generated)
kwiberg@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc File webrtc/base/asyncudpsocket.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); The double not is not always not so readable, except that here it is not. Compare to nullptr instead or even just check socket? (no underscore) Alternatively fix ctor to use unique_ptr. Actually, the pointer is dereferenced below, always, so this isn't necessary. https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/httpserver.cc File webrtc/base/httpserver.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/httpserver.cc#new... webrtc/base/httpserver.cc:271: ASSERT(!!listener_); Not necessary https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:70: Logger::log_ = s.get(); Wow... Can this just return a vector instead? (move) https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:237: bool reconnect = !!dtls_; My preference is to not not not the dtls.
https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc File webrtc/base/asyncudpsocket.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); On 2016/03/15 09:22:09, tommi-webrtc wrote: > The double not is not always not so readable, except that here it is not. > Compare to nullptr instead or even just check socket? (no underscore) > Alternatively fix ctor to use unique_ptr. > > Actually, the pointer is dereferenced below, always, so this isn't necessary. OK, will remove. (!!x is the standard C idiom for converting x to a bool---it's only confusing the first time you see it. I guess in C++ it's more customary to use bool(x) or static_cast<bool>(x)...) https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/httpserver.cc File webrtc/base/httpserver.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/httpserver.cc#new... webrtc/base/httpserver.cc:271: ASSERT(!!listener_); On 2016/03/15 09:22:09, tommi-webrtc wrote: > Not necessary Done. https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:70: Logger::log_ = s.get(); On 2016/03/15 09:22:09, tommi-webrtc wrote: > Wow... Can this just return a vector instead? (move) Yeah, I was about to send the author a sharply-worded note for not doing that in the first place, but then I looked at the name of this file and realized who must have written it... Short version: It's done like this for a good reason. A pointer to the returned vector is saved in a static member variable, where all Logger instances can get at it (to write to the log). Each unit test begins by calling Setup, then creates one or more Loggers that live and die, and at the end, the unit test checks the log. Of course, this ought to have been done with shared_ptr and weak_ptr rather than manually making sure that the raw pointer to the vector isn't used after the scoped_ptr goes out of scope, but the unit tests are small and simple. https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:237: bool reconnect = !!dtls_; On 2016/03/15 09:22:09, tommi-webrtc wrote: > My preference is to not not not the dtls. You complained about this an even number of times. Does that mean I don't have to change it? ;-) Changed to static_cast<bool>.
tommi@chromium.org changed reviewers: + tommi@chromium.org
https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc File webrtc/base/asyncudpsocket.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > The double not is not always not so readable, except that here it is not. > > Compare to nullptr instead or even just check socket? (no underscore) > > Alternatively fix ctor to use unique_ptr. > > > > Actually, the pointer is dereferenced below, always, so this isn't necessary. > > OK, will remove. (!!x is the standard C idiom for converting x to a bool---it's > only confusing the first time you see it. I guess in C++ it's more customary to > use bool(x) or static_cast<bool>(x)...) This isn't the first time I'm seeing it :) https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:70: Logger::log_ = s.get(); On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > Wow... Can this just return a vector instead? (move) > > Yeah, I was about to send the author a sharply-worded note for not doing that in > the first place, but then I looked at the name of this file and realized who > must have written it... > > Short version: It's done like this for a good reason. A pointer to the returned > vector is saved in a static member variable, where all Logger instances can get > at it (to write to the log). Each unit test begins by calling Setup, then > creates one or more Loggers that live and die, and at the end, the unit test > checks the log. > > Of course, this ought to have been done with shared_ptr and weak_ptr rather than > manually making sure that the raw pointer to the vector isn't used after the > scoped_ptr goes out of scope, but the unit tests are small and simple. OK. Btw we shouldn't use auto when it actually makes things more verbose: auto s = rtc::scoped_ptr<std::vector<std::string>>(new std::vector<std::string>); vs rtc::scoped_ptr<std::vector<std::string>> s(new std::vector<std::string>); It is very confusing that "Logger::log_" is a reference to a global variable. I didn't actually even notice the underscore at first. To me it looked like a variable declaration at first glance. It should be just g_log (or s_log but I think g_ is more commonly used) and since we're in the Logger class, we don't have to further qualify the variable. Same for next_id_. Since this is static though and there's a global pointer kept that points to something that's being returned via a scoped_ptr, I think that's not a good design choice. Feels like asking for dangling pointers to me. Either the ownership lies elsewhere entirely or it lies with this class entirely. If here, then the class could expose a TearDown method to free the singleton. Also, I don't see a check for overwriting a possible previous value of log_. Since the tests rely on global data, we need to be careful to catch potentially bad state left by previous runs (I'm assuming it's not possible to run these tests in parallel as is). If log_ is ever not null when we assign to it, we should crash. Anyway, getting back on track for this particular review, we can leave it as is but feel free to remove the auto. https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:237: bool reconnect = !!dtls_; On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > My preference is to not not not the dtls. > > You complained about this an even number of times. Does that mean I don't have > to change it? ;-) > > Changed to static_cast<bool>. I'd rather compare dtls_ to a value of the same type that dtls_ is. Since dtls_ represents a pointer, I'd rather do: bool reconnect = (dtls_.get() != nullptr);
New patch set posted. https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc File webrtc/base/asyncudpsocket.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); On 2016/03/15 16:49:00, tommi-chromium wrote: > On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > > The double not is not always not so readable, except that here it is not. > > > Compare to nullptr instead or even just check socket? (no underscore) > > > Alternatively fix ctor to use unique_ptr. > > > > > > Actually, the pointer is dereferenced below, always, so this isn't > necessary. > > > > OK, will remove. (!!x is the standard C idiom for converting x to a > bool---it's > > only confusing the first time you see it. I guess in C++ it's more customary > to > > use bool(x) or static_cast<bool>(x)...) > > This isn't the first time I'm seeing it :) And yet you acted confused... https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:70: Logger::log_ = s.get(); On 2016/03/15 16:49:00, tommi-chromium wrote: > On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > > Wow... Can this just return a vector instead? (move) > > > > Yeah, I was about to send the author a sharply-worded note for not doing that > in > > the first place, but then I looked at the name of this file and realized who > > must have written it... > > > > Short version: It's done like this for a good reason. A pointer to the > returned > > vector is saved in a static member variable, where all Logger instances can > get > > at it (to write to the log). Each unit test begins by calling Setup, then > > creates one or more Loggers that live and die, and at the end, the unit test > > checks the log. > > > > Of course, this ought to have been done with shared_ptr and weak_ptr rather > than > > manually making sure that the raw pointer to the vector isn't used after the > > scoped_ptr goes out of scope, but the unit tests are small and simple. > > OK. Btw we shouldn't use auto when it actually makes things more verbose: > auto s = rtc::scoped_ptr<std::vector<std::string>>(new > std::vector<std::string>); > vs > rtc::scoped_ptr<std::vector<std::string>> s(new std::vector<std::string>); Yeah, thanks for catching that. Fixed. > It is very confusing that "Logger::log_" is a reference to a global variable. I > didn't actually even notice the underscore at first. To me it looked like a > variable declaration at first glance. It should be just g_log (or s_log but I > think g_ is more commonly used) and since we're in the Logger class, we don't > have to further qualify the variable. > Same for next_id_. Sounds like all good ideas. > Since this is static though and there's a global pointer kept that points to > something that's being returned via a scoped_ptr, I think that's not a good > design choice. Feels like asking for dangling pointers to me. Yes. You mustn't forget to call Setup(). But without calling it you don't have a log to verify, so it's not hard to remember. > Either the > ownership lies elsewhere entirely or it lies with this class entirely. If here, > then the class could expose a TearDown method to free the singleton. But then we'd have to remember calling that instead. > Also, I don't see a check for overwriting a possible previous value of log_. True. > Since the > tests rely on global data, we need to be careful to catch potentially bad state > left by previous runs (I'm assuming it's not possible to run these tests in > parallel as is). If log_ is ever not null when we assign to it, we should > crash. It's going to be non-null for all but the first test case to run in the same process, isn't it? Overwriting is harmless as long as previous runs have finished, since the static pointer isn't the owner. Unfortunately we need the static pointer to the log, since we can't pass it as a constructor argument. But I guess if we used a scoped_ptr with a custom deleter, that deleter could check that the static pointer still pointed to the right thing, then set it to null; Setup could then check that the static pointer started out as null. That's the safest I can think of. > Anyway, getting back on track for this particular review, we can leave it as is > but feel free to remove the auto. https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/p2p/base/dtlstransport... webrtc/p2p/base/dtlstransportchannel.cc:237: bool reconnect = !!dtls_; On 2016/03/15 16:49:00, tommi-chromium wrote: > On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > > My preference is to not not not the dtls. > > > > You complained about this an even number of times. Does that mean I don't have > > to change it? ;-) > > > > Changed to static_cast<bool>. > > I'd rather compare dtls_ to a value of the same type that dtls_ is. Since dtls_ > represents a pointer, I'd rather do: > > bool reconnect = (dtls_.get() != nullptr); Still not convinced this is more readable, but OK. (It turns out we don't need the .get(), however---unique_ptr's operator!= accepts nullptr_t arguments.)
lgtm https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc File webrtc/base/asyncudpsocket.cc (right): https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); On 2016/03/15 18:53:37, kwiberg-webrtc wrote: > On 2016/03/15 16:49:00, tommi-chromium wrote: > > On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > > > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > > > The double not is not always not so readable, except that here it is not. > > > > Compare to nullptr instead or even just check socket? (no underscore) > > > > Alternatively fix ctor to use unique_ptr. > > > > > > > > Actually, the pointer is dereferenced below, always, so this isn't > > necessary. > > > > > > OK, will remove. (!!x is the standard C idiom for converting x to a > > bool---it's > > > only confusing the first time you see it. I guess in C++ it's more customary > > to > > > use bool(x) or static_cast<bool>(x)...) > > > > This isn't the first time I'm seeing it :) > > And yet you acted confused... I guess I was rhetorically confused :) https://codereview.webrtc.org/1803833002/diff/40001/webrtc/base/optional_unit... File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/1803833002/diff/40001/webrtc/base/optional_unit... webrtc/base/optional_unittest.cc:69: Logger::log_ = s.get(); Do you mind removing the "Logger::" prefix where it's not necessary in this file? Would also be good to put down a TODO to rename the log_ and next_id_ variables to g_log and g_next_id.
On 2016/03/17 12:23:26, tommi-webrtc wrote: > lgtm > > https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc > File webrtc/base/asyncudpsocket.cc (right): > > https://codereview.webrtc.org/1803833002/diff/1/webrtc/base/asyncudpsocket.cc... > webrtc/base/asyncudpsocket.cc:40: ASSERT(!!socket_); > On 2016/03/15 18:53:37, kwiberg-webrtc wrote: > > On 2016/03/15 16:49:00, tommi-chromium wrote: > > > On 2016/03/15 09:59:41, kwiberg-webrtc wrote: > > > > On 2016/03/15 09:22:09, tommi-webrtc wrote: > > > > > The double not is not always not so readable, except that here it is > not. > > > > > Compare to nullptr instead or even just check socket? (no underscore) > > > > > Alternatively fix ctor to use unique_ptr. > > > > > > > > > > Actually, the pointer is dereferenced below, always, so this isn't > > > necessary. > > > > > > > > OK, will remove. (!!x is the standard C idiom for converting x to a > > > bool---it's > > > > only confusing the first time you see it. I guess in C++ it's more > customary > > > to > > > > use bool(x) or static_cast<bool>(x)...) > > > > > > This isn't the first time I'm seeing it :) > > > > And yet you acted confused... > > I guess I was rhetorically confused :) > > https://codereview.webrtc.org/1803833002/diff/40001/webrtc/base/optional_unit... > File webrtc/base/optional_unittest.cc (right): > > https://codereview.webrtc.org/1803833002/diff/40001/webrtc/base/optional_unit... > webrtc/base/optional_unittest.cc:69: Logger::log_ = s.get(); > Do you mind removing the "Logger::" prefix where it's not necessary in this > file? > Would also be good to put down a TODO to rename the log_ and next_id_ variables > to g_log and g_next_id. I'll make a separate CL.
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/9593) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6035) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/6025) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8363) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8277) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13561) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12193) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13300) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/585) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13217) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7906) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13596)
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1803833002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/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) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4222)
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1803833002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803833002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803833002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Stop using some scoped_ptr features that unique_ptr doesn't have No operator== that accepts one unique_ptr<T> and one T*. No implicit conversion to bool. No rtc_make_scoped_ptr function. BUG=webrtc:5520 ========== to ========== Stop using some scoped_ptr features that unique_ptr doesn't have No operator== that accepts one unique_ptr<T> and one T*. No implicit conversion to bool. No rtc_make_scoped_ptr function. BUG=webrtc:5520 Committed: https://crrev.com/1d50ee44fd53e228d40db2d6aa2f0300b736c550 Cr-Commit-Position: refs/heads/master@{#12048} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1d50ee44fd53e228d40db2d6aa2f0300b736c550 Cr-Commit-Position: refs/heads/master@{#12048} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
