Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2181053002:
android: dcheck_always_on sets BuildConfig.IS_DCHECK (Closed)
Descriptionandroid: dcheck_always_on sets BuildConfig.DCHECK_IS_ON
Better matches with native code.
Native code turns on DCHECKs with dcheck_always_on.
All usage of BuildConfig.IS_DEBUG is really guarding
DCHECK equivalents in Java. So rename the variable
DCHECK_IS_ON, and make it depend on dcheck_always_on
as well.
Committed: https://crrev.com/cd51f6c855c7bb95bfcffefc7827f6a941a498fe
Cr-Commit-Position: refs/heads/master@{#408265}
Patch Set 1 #Patch Set 2 : rename #Patch Set 3 : fix findbugs #
Messages
Total messages: 38 (18 generated)
boliu@chromium.org changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org
agree? I have no idea why there are two copies of the template rule, super weird..
I don't think this is the right thing to do, but I'm not sure what your motivation here is. I'm not immediately sure why the rule is in there twice.
On 2016/07/25 21:23:24, jbudorick wrote: > I don't think this is the right thing to do, but I'm not sure what your > motivation here is. > > I'm not immediately sure why the rule is in there twice. Generally dcheck_always_on means DCHECKs are turned on native code. But right now dcheck_always_on does not set on BuildConfig.IS_DEBUG, and some java exceptions/asserts are behind BuildConfig.IS_DEBUG right now. Maybe should rename IS_DEBUG to DCHECK_IS_ON, matching this CL: https://codereview.chromium.org/2163023002/
On 2016/07/25 21:25:14, boliu wrote: > On 2016/07/25 21:23:24, jbudorick wrote: > > I don't think this is the right thing to do, but I'm not sure what your > > motivation here is. > > > > I'm not immediately sure why the rule is in there twice. > > Generally dcheck_always_on means DCHECKs are turned on native code. But right :/ > now dcheck_always_on does not set on BuildConfig.IS_DEBUG, and some java > exceptions/asserts are behind BuildConfig.IS_DEBUG right now. dcheck_always_on != debug. If java code wants to check if DCHECKs are on, fine, let's add that to BuildConfig. But if they're checking for debug or release, they should get exactly that, especially given that we use release w/ dchecks on the trybots. > > Maybe should rename IS_DEBUG to DCHECK_IS_ON, matching this CL: > https://codereview.chromium.org/2163023002/
On 2016/07/25 21:27:02, jbudorick wrote: > On 2016/07/25 21:25:14, boliu wrote: > > On 2016/07/25 21:23:24, jbudorick wrote: > > > I don't think this is the right thing to do, but I'm not sure what your > > > motivation here is. > > > > > > I'm not immediately sure why the rule is in there twice. > > > > Generally dcheck_always_on means DCHECKs are turned on native code. But right > > :/ > > > now dcheck_always_on does not set on BuildConfig.IS_DEBUG, and some java > > exceptions/asserts are behind BuildConfig.IS_DEBUG right now. > > dcheck_always_on != debug. > > If java code wants to check if DCHECKs are on, fine, let's add that to > BuildConfig. But if they're checking for debug or release, they should get > exactly that, especially given that we use release w/ dchecks on the trybots. All existing usage of IS_DEBUG should be replaced with DCHECK_IS_ON, so it ends up being just a rename anyway > > > > > Maybe should rename IS_DEBUG to DCHECK_IS_ON, matching this CL: > > https://codereview.chromium.org/2163023002/
On 2016/07/25 21:29:53, boliu wrote: > On 2016/07/25 21:27:02, jbudorick wrote: > > On 2016/07/25 21:25:14, boliu wrote: > > > On 2016/07/25 21:23:24, jbudorick wrote: > > > > I don't think this is the right thing to do, but I'm not sure what your > > > > motivation here is. > > > > > > > > I'm not immediately sure why the rule is in there twice. > > > > > > Generally dcheck_always_on means DCHECKs are turned on native code. But > right > > > > :/ > > > > > now dcheck_always_on does not set on BuildConfig.IS_DEBUG, and some java > > > exceptions/asserts are behind BuildConfig.IS_DEBUG right now. > > > > dcheck_always_on != debug. > > > > If java code wants to check if DCHECKs are on, fine, let's add that to > > BuildConfig. But if they're checking for debug or release, they should get > > exactly that, especially given that we use release w/ dchecks on the trybots. > > All existing usage of IS_DEBUG should be replaced with DCHECK_IS_ON, so it ends > up being just a rename anyway > > > > > > > > > Maybe should rename IS_DEBUG to DCHECK_IS_ON, matching this CL: > > > https://codereview.chromium.org/2163023002/ I'm fine with renaming it and changing the define, then.
Description was changed from ========== android: dcheck_always_on sets BuildConfig.IS_DEBUG Better matches with C++ code, which better matches most native code. ========== to ========== android: dcheck_always_on sets BuildConfig.IS_DEBUG Better matches with native code. Native code turns out DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ==========
On 2016/07/25 21:31:36, jbudorick wrote: > On 2016/07/25 21:29:53, boliu wrote: > > On 2016/07/25 21:27:02, jbudorick wrote: > > > On 2016/07/25 21:25:14, boliu wrote: > > > > On 2016/07/25 21:23:24, jbudorick wrote: > > > > > I don't think this is the right thing to do, but I'm not sure what your > > > > > motivation here is. > > > > > > > > > > I'm not immediately sure why the rule is in there twice. > > > > > > > > Generally dcheck_always_on means DCHECKs are turned on native code. But > > right > > > > > > :/ > > > > > > > now dcheck_always_on does not set on BuildConfig.IS_DEBUG, and some java > > > > exceptions/asserts are behind BuildConfig.IS_DEBUG right now. > > > > > > dcheck_always_on != debug. > > > > > > If java code wants to check if DCHECKs are on, fine, let's add that to > > > BuildConfig. But if they're checking for debug or release, they should get > > > exactly that, especially given that we use release w/ dchecks on the > trybots. > > > > All existing usage of IS_DEBUG should be replaced with DCHECK_IS_ON, so it > ends > > up being just a rename anyway > > > > > > > > > > > > > Maybe should rename IS_DEBUG to DCHECK_IS_ON, matching this CL: > > > > https://codereview.chromium.org/2163023002/ > > I'm fine with renaming it and changing the define, then. renamed, let's see what bots say about this
boliu@chromium.org changed reviewers: + yfriedman@chromium.org
bot be green! +yfriedman for all the non-build things I touched
lgtm, but please update the commit title.
Description was changed from ========== android: dcheck_always_on sets BuildConfig.IS_DEBUG Better matches with native code. Native code turns out DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ========== to ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns out DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ==========
On 2016/07/25 23:42:05, jbudorick wrote: > lgtm, but please update the commit title. done
On 2016/07/25 23:46:28, boliu wrote: > On 2016/07/25 23:42:05, jbudorick wrote: > > lgtm, but please update the commit title. > > done lgtm
boliu@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist for review of base and chrome, since yaron is ooo
lgtm, but could you clarify the CL description a bit? This sentence is confusing: "Native code turns out DCHECKs with dcheck_always_on."
Description was changed from ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns out DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ========== to ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns on DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ==========
On 2016/07/27 16:46:52, nyquist wrote: > lgtm, but could you clarify the CL description a bit? > This sentence is confusing: "Native code turns out DCHECKs with > dcheck_always_on." err, typo, s/out/on/ is that clear..?
boliu@chromium.org changed reviewers: + danakj@chromium.org
+danakj for stamping base/BUILD.gn
On 2016/07/27 16:48:28, boliu wrote: > On 2016/07/27 16:46:52, nyquist wrote: > > lgtm, but could you clarify the CL description a bit? > > This sentence is confusing: "Native code turns out DCHECKs with > > dcheck_always_on." > > err, typo, s/out/on/ is that clear..? Yeah, that's fine.
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
BUILD LGTM
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2181053002/#ps40001 (title: "fix findbugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns on DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ========== to ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns on DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns on DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. ========== to ========== android: dcheck_always_on sets BuildConfig.DCHECK_IS_ON Better matches with native code. Native code turns on DCHECKs with dcheck_always_on. All usage of BuildConfig.IS_DEBUG is really guarding DCHECK equivalents in Java. So rename the variable DCHECK_IS_ON, and make it depend on dcheck_always_on as well. Committed: https://crrev.com/cd51f6c855c7bb95bfcffefc7827f6a941a498fe Cr-Commit-Position: refs/heads/master@{#408265} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cd51f6c855c7bb95bfcffefc7827f6a941a498fe Cr-Commit-Position: refs/heads/master@{#408265} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
