Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731263003:
Allow constexpr function in MSVC (Closed)
DescriptionAllow constexpr function in MSVC
BUG=NO
Review-Url: https://codereview.chromium.org/2731263003
Cr-Commit-Position: refs/heads/master@{#43636}
Committed: https://chromium.googlesource.com/v8/v8/+/8038f1bd678065e295686c80eb4c394776e75268
Patch Set 1 #Patch Set 2 : drop globals.h #
Messages
Total messages: 28 (15 generated)
Description was changed from ========== More constexpr BUG=NO ========== to ========== More constexpr BUG=NO ==========
loorongjie@gmail.com changed reviewers: + clemensh@chromium.org, vogelheim@chromium.org
The CQ bit was checked by vogelheim@google.com 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_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18541)
Thanks for the patch! The MSVC issue in token.cc seems to be resolved, but several of the (Linux) bots are unhappy about the globals.h changes. Examples: - https://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_re... - https://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/buil... - https://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_... Off-hand, I'm not sure whether that applies to all of the constexprs, or only some of them. Also, I'll try to find out (today or tomorrow-ish) whether the compiler versions on commit queue represent the baseline compilers we need to support. (That is: Can we assume that 'passes on cq' == 'ok', or do we need to support compiler versions that are older than what we run in the cq right now.)
lgtm for src/parsing/token.cc (only). I'm a bit unsure how to proceed with globals.h. One option would be to just drop those changes entirely; another would be to restrict it to only those expressions that are constexpr-able on all supported compilers. (reinterpret_cast<> appears to be the main offender.)
jochen@chromium.org changed reviewers: + jochen@chromium.org
http://chromium-cpp.appspot.com/ says "Don't go out of the way to convert existing code." about constexpr, so I'd lean against this
On 2017/03/07 at 10:32:39, jochen wrote: > http://chromium-cpp.appspot.com/ says "Don't go out of the way to convert existing code." about constexpr, so I'd lean against this Oh, that's how you read that phrase. Maybe we should rephrase it for non-native readers. I read it as "Don't hesitate to ...". So would you also object to this CL? https://chromium-review.googlesource.com/c/451277/
On 2017/03/07 10:32:39, jochen wrote: > http://chromium-cpp.appspot.com/ says "Don't go out of the way to convert > existing code." about constexpr, so I'd lean against this OK. Still 'lgtm' for src/parsing/token.cc, then, but I'd prefer to drop the changes in src/globals.h.
On 2017/03/07 at 10:50:33, clemensh wrote: > On 2017/03/07 at 10:32:39, jochen wrote: > > http://chromium-cpp.appspot.com/ says "Don't go out of the way to convert existing code." about constexpr, so I'd lean against this > > Oh, that's how you read that phrase. Maybe we should rephrase it for non-native readers. I read it as "Don't hesitate to ...". > So would you also object to this CL? https://chromium-review.googlesource.com/c/451277/ I dunno, in that other CL, you seem to have a concrete use case for doing the conversion
Dropped src\globals.h , doesn't really matter as a decent compiler should be able to to constant folding and inlining even without constexpr anyway.
Description was changed from ========== More constexpr BUG=NO ========== to ========== Allow constexpr function in MSVC BUG=NO ==========
The CQ bit was checked by vogelheim@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...
lgtm
lgtm I think in general it is desirable to transform more code to constexpr, but only if there is any actual value of doing it. And only where it's standard-compliant of course, reinterpret_cast for example is explicitely disallowed. So once you want to introduce new constexpr variables/fields, but this depends on other stuff being constexpr, feel free to change that other code.
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 vogelheim@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": 20001, "attempt_start_ts": 1488887676984260,
"parent_rev": "83b96b58329889c001ce0aa8173ff52986677797", "commit_rev":
"8038f1bd678065e295686c80eb4c394776e75268"}
Message was sent while issue was closed.
Description was changed from ========== Allow constexpr function in MSVC BUG=NO ========== to ========== Allow constexpr function in MSVC BUG=NO Review-Url: https://codereview.chromium.org/2731263003 Cr-Commit-Position: refs/heads/master@{#43636} Committed: https://chromium.googlesource.com/v8/v8/+/8038f1bd678065e295686c80eb4c394776e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/8038f1bd678065e295686c80eb4c394776e...
Message was sent while issue was closed.
On 2017/03/07 11:56:25, commit-bot: I haz the power wrote: > Committed patchset #2 (id:20001) as > https://chromium.googlesource.com/v8/v8/+/8038f1bd678065e295686c80eb4c394776e... clemens@ You are right about reinterpret_cast is forbidden for constexpr, VS2017 RTM throws error now. https://twitter.com/StephanTLavavej/status/839365305310699521 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
