Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213623017:
clang/win: Let the compiler know that NaClSwitch() cannot return. (Closed)
|
Created:
5 years, 5 months ago by Nico Modified:
5 years, 5 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
Descriptionclang/win: Let the compiler know that NaClSwitch() cannot return.
No intended behavior change, this is to make -Winvalid-noreturn happy
on Windows.
BUG=chromium:504698
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/d343e0729ef198620d0446e0b17640b5461c0687
Patch Set 1 #
Total comments: 5
Patch Set 2 : comments #Messages
Total messages: 28 (11 generated)
thakis@chromium.org changed reviewers: + mseaborn@chromium.org
LGTM https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c (right): https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c:74: #if NACL_WINDOWS && __clang__ This should be "#if NACL_WINDOWS && defined(__clang__)" https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c:75: /* NaClSwitch is a function pointer in x86_32 and there's no way to mark Nit: please use the NaCl style for multiline comments, with "/*" and "*/" on separate lines, like this: /* * Blah... * blah. */
Thanks! https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c (right): https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c:74: #if NACL_WINDOWS && __clang__ On 2015/07/07 21:05:04, Mark Seaborn wrote: > This should be "#if NACL_WINDOWS && defined(__clang__)" Done. (Both works, undefined symbols evaluate to 0) https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c:75: /* NaClSwitch is a function pointer in x86_32 and there's no way to mark On 2015/07/07 21:05:04, Mark Seaborn wrote: > Nit: please use the NaCl style for multiline comments, with "/*" and "*/" on > separate lines, like this: > /* > * Blah... > * blah. > */ Done.
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/1213623017/#ps20001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213623017/20001
https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c (right): https://codereview.chromium.org/1213623017/diff/1/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/nacl_switch_to_app_32.c:74: #if NACL_WINDOWS && __clang__ On 2015/07/07 21:10:46, Nico wrote: > On 2015/07/07 21:05:04, Mark Seaborn wrote: > > This should be "#if NACL_WINDOWS && defined(__clang__)" > > Done. (Both works, undefined symbols evaluate to 0) Yeah, but we compile with -Wundef on compilers that support it. See: http://lackingrhoticity.blogspot.com/2015/01/conditionalising-c-ifdef-vs-if.html
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213623017/20001
On 2015/07/07 21:53:09, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1213623017/20001 The CQ looks pretty confused. There are like hundreds of green bots though – can you land this for me? (I tried `git cl land` but apparently I don't have a nacl commit bit anymore in this new git world)
On 7 July 2015 at 17:16, <thakis@chromium.org> wrote: > On 2015/07/07 21:53:09, commit-bot: I haz the power wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-cq-status.appspot.com/patch-status/1213623017/20001 >> > > The CQ looks pretty confused. There are like hundreds of green bots though > – can > you land this for me? (I tried `git cl land` but apparently I don't have a > nacl > commit bit anymore in this new git world) Let's give it a bit longer -- the CQ has been committing NaCl changes OK recently, albeit slowly. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
On 2015/07/08 00:38:59, commit-bot: I haz the power wrote: > Exceeded global retry quota Looks like it just failed again. Want me to verify it a third time?
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213623017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213623017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: nacl-arm_opt_panda on tryserver.nacl (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_opt_panda/builds...)
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213623017/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/native_client/src/native_client/+/d343e0729... |
