Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524093002:
[base] Pass scalar arguments by value in CHECK/DCHECK (Closed)
|
Created:
4 years ago by Clemens Hammacher Modified:
4 years ago CC:
Benedikt Meurer, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[base] Pass scalar arguments by value in CHECK/DCHECK
This not only potentially improves performance, but also avoids weird
linker errors, like the one below, where I used Smi::kMinValue in a
DCHECK_LE.
> [421/649] LINK ./mksnapshot
> FAILED: mksnapshot
> src/base/logging.h|178| error: undefined reference to
'v8::internal::Smi::kMinValue'
R=bmeurer@chromium.org, ishell@chromium.org
Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3
Committed: https://crrev.com/8fcfe66f9458e2dd1009d4854786aec428d5cc59
Cr-Original-Commit-Position: refs/heads/master@{#41273}
Cr-Commit-Position: refs/heads/master@{#41363}
Patch Set 1 #Patch Set 2 : Avoid constexpr for our VS2013 windows bots #Patch Set 3 : Replace typeof by decltype #Patch Set 4 : Help Android to determine the type correctly #Patch Set 5 : Add std::decay before checking for std::scalar #Patch Set 6 : Remove unneeded const #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 49 (37 generated)
The CQ bit was checked by clemensh@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: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18323) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18438)
The CQ bit was checked by clemensh@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: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13101)
The CQ bit was checked by clemensh@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: This issue passed the CQ dry run.
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but can also avoid weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' ==========
clemensh@chromium.org changed reviewers: + bmeurer@chromium.org
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org ==========
Ah, yeah, that makes sense. Thanks. LGTM.
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=ishell@chromium.org ==========
clemensh@chromium.org changed reviewers: + ishell@chromium.org - bmeurer@chromium.org
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=ishell@chromium.org ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org ==========
clemensh@chromium.org changed reviewers: + bmeurer@chromium.org
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480007527415360,
"parent_rev": "df884a6751a01d18cfafcc5220116cd0d4159c89", "commit_rev":
"7b1ac2b736df5478f10e5123a7f8aeb47d4f5760"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Cr-Commit-Position: refs/heads/master@{#41273} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Cr-Commit-Position: refs/heads/master@{#41273}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2527883004/ by clemensh@chromium.org. The reason for reverting is: Seems to cause compile errors on Android. Will investigate on Monday. .
The CQ bit was checked by clemensh@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 checked by clemensh@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...
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_EQ. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Cr-Commit-Position: refs/heads/master@{#41273} ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_LE. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Cr-Commit-Position: refs/heads/master@{#41273} ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@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: This issue passed the CQ dry run.
I added std::decay type conversion, which did somehow not happen automatically on android builds before (they failed at the compile step). PTAL.
lgtm https://codereview.chromium.org/2524093002/diff/100001/src/base/logging.h File src/base/logging.h (right): https://codereview.chromium.org/2524093002/diff/100001/src/base/logging.h#new... src/base/logging.h:84: typename std::decay<T>::type, T const&> {}; Awesome! Does it mean that we can now move definition of Smi::kZero to header?
On 2016/11/29 at 14:06:05, ishell wrote: > lgtm > > https://codereview.chromium.org/2524093002/diff/100001/src/base/logging.h > File src/base/logging.h (right): > > https://codereview.chromium.org/2524093002/diff/100001/src/base/logging.h#new... > src/base/logging.h:84: typename std::decay<T>::type, T const&> {}; > Awesome! Does it mean that we can now move definition of Smi::kZero to header? I tried it: Did not work before this CL, but works afterwards. It requires the constexpr annotation though, and I am not sure whether our windows bots are fine with it. Let's try it in another CL, and postpone until Michael landed the switch to VS2015 (looks like we are close now!)
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2524093002/#ps100001 (title: "Remove unneeded const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480429901760020,
"parent_rev": "11cd475d4b7f5039623132909b7542ee90a48b9b", "commit_rev":
"a285bd7c3fdb978d7186aef51881b5c0556b4d58"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_LE. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Cr-Commit-Position: refs/heads/master@{#41273} ========== to ========== [base] Pass scalar arguments by value in CHECK/DCHECK This not only potentially improves performance, but also avoids weird linker errors, like the one below, where I used Smi::kMinValue in a DCHECK_LE. > [421/649] LINK ./mksnapshot > FAILED: mksnapshot > src/base/logging.h|178| error: undefined reference to 'v8::internal::Smi::kMinValue' R=bmeurer@chromium.org, ishell@chromium.org Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3 Committed: https://crrev.com/8fcfe66f9458e2dd1009d4854786aec428d5cc59 Cr-Original-Commit-Position: refs/heads/master@{#41273} Cr-Commit-Position: refs/heads/master@{#41363} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8fcfe66f9458e2dd1009d4854786aec428d5cc59 Cr-Commit-Position: refs/heads/master@{#41363} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
