Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647663002:
Delete or update left-over ASSERT use and comments. (Closed)
|
Created:
3 years, 11 months ago by nisse-webrtc Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, zhengzhonghou_agora.io, tterriberry_mozilla.com, sdk-team_agora.io, the sun, perkj_webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete or update left-over ASSERT use and comments.
BUG=webrtc:6424, webrtc:6323
Review-Url: https://codereview.webrtc.org/2647663002
Cr-Commit-Position: refs/heads/master@{#16215}
Committed: https://chromium.googlesource.com/external/webrtc/+/0ebdf2757c585f04eba18c9b4e7dd68910c42e35
Patch Set 1 #
Total comments: 5
Messages
Total messages: 25 (10 generated)
nisse@webrtc.org changed reviewers: + kjellander@webrtc.org, kwiberg@webrtc.org
@kjellander, regarding the BUILD.gn change: The _DEBUG define is unused as far as I see, but I could be missing something.
lgtm I don't know C/C++ well enough to tell if this has impact on system library headers etc, that might be looking at the define.
On 2017/01/19 14:58:57, kjellander_webrtc wrote: > lgtm > I don't know C/C++ well enough to tell if this has impact on system library > headers etc, that might be looking at the define. I have never heard of a standard meaning of _DEBUG earlier, but a google search indicates that it is used by Microsoft's compiler. See http://stackoverflow.com/questions/2290509/debug-vs-ndebug (half-decade old so might be outdated, though). I don't know if we use any of these Microsoft-specific debug features, but the comments in BUILD.gn seem to indicate that collision with the compiler's use is accidental. But if we do, I suspect it's better to enable them on the compilation command line using /MTd /MDd or whatever, instead of trying to override _DEBUG.
On 2017/01/23 12:10:51, nisse-webrtc wrote: > On 2017/01/19 14:58:57, kjellander_webrtc wrote: > > lgtm > > I don't know C/C++ well enough to tell if this has impact on system library > > headers etc, that might be looking at the define. > > I have never heard of a standard meaning of _DEBUG earlier, but a google search > indicates that it is used by Microsoft's compiler. See > http://stackoverflow.com/questions/2290509/debug-vs-ndebug (half-decade old so > might be outdated, though). > > I don't know if we use any of these Microsoft-specific debug features, but the > comments in BUILD.gn seem to indicate that collision with the compiler's use is > accidental. > > But if we do, I suspect it's better to enable them on the compilation command > line using /MTd /MDd or whatever, instead of trying to override _DEBUG. Thanks for taking the time to investigate. I'd say go ahead and submit and we'll see. You could also just check real quick with a Windows guru like Tommi.
lgtm (The comments below duplicate some of yours; I wrote these yesterday but forgot to actually post them.) https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (left): https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/BUILD.gn#oldcode673 webrtc/base/BUILD.gn:673: } Curiously, _DEBUG seems to be a Windows thing, not a posix thing as implied by this code: http://stackoverflow.com/questions/2290509/debug-vs-ndebug I'm not sure if we need it or not. Did we use it before? E.g. was ASSERT defined in terms of _DEBUG as the comment claims? https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h#newco... webrtc/base/fileutils.h:96: // non-existent file. Not your fault, but I'll just point out that this behavior conflicts with the "don't handle DCHECK errors" rule.
https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (left): https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/BUILD.gn#oldcode673 webrtc/base/BUILD.gn:673: } On 2017/01/23 12:25:14, kwiberg-webrtc wrote: > Curiously, _DEBUG seems to be a Windows thing, not a posix thing as implied by > this code: http://stackoverflow.com/questions/2290509/debug-vs-ndebug > > I'm not sure if we need it or not. Did we use it before? E.g. was ASSERT defined > in terms of _DEBUG as the comment claims? I've search history, _DEBUG was used in lots of places before cl https://codereview.webrtc.org/1429513004 (but not for ASSERT, as far as I see). More recently, it was used in peerconnection_unittests.cc, and that use deleted in cl https://codereview.webrtc.org/1837393002 https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h#newco... webrtc/base/fileutils.h:96: // non-existent file. On 2017/01/23 12:25:14, kwiberg-webrtc wrote: > Not your fault, but I'll just point out that this behavior conflicts with the > "don't handle DCHECK errors" rule. When I get to erplacing use of VERIFY, there's a common pattern to check if an operation fails, and if so, trigger an abort in debug builds, but return some failure value in non-debug builds. See, e.g., https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=VERIF... What do you suggest I do about those?
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17901) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15445) win_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5506) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21908)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17903) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10087) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15447) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22406) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5508) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2647663002/diff/1/webrtc/base/fileutils.h#newco... webrtc/base/fileutils.h:96: // non-existent file. On 2017/01/23 12:53:03, nisse-webrtc wrote: > On 2017/01/23 12:25:14, kwiberg-webrtc wrote: > > Not your fault, but I'll just point out that this behavior conflicts with the > > "don't handle DCHECK errors" rule. > > When I get to erplacing use of VERIFY, there's a common pattern to check if an > operation fails, and if so, trigger an abort in debug builds, but return some > failure value in non-debug builds. > > See, e.g., > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=VERIF... > > What do you suggest I do about those? I don't know. But I do know that DCHECK is the wrong thing to use for such situations. Personally, I lean towards not doing that at all, since the error handling path is per construction difficult to test. But I realize this would make your search-and-replace difficult... Maybe post the first post in what's sure to be a centithread on e.g. webrtc-eng@?
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17910) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10096) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15454) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22414) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1485184553196660, "parent_rev":
"da250064312d4ad6404ac962fc0ef33b6359bb72", "commit_rev":
"0ebdf2757c585f04eba18c9b4e7dd68910c42e35"}
Message was sent while issue was closed.
Description was changed from ========== Delete or update left-over ASSERT use and comments. BUG=webrtc:6424,webrtc:6323 ========== to ========== Delete or update left-over ASSERT use and comments. BUG=webrtc:6424,webrtc:6323 Review-Url: https://codereview.webrtc.org/2647663002 Cr-Commit-Position: refs/heads/master@{#16215} Committed: https://chromium.googlesource.com/external/webrtc/+/0ebdf2757c585f04eba18c9b4... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/0ebdf2757c585f04eba18c9b4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
