Deduplicates NaClSysSysconf implementation

Deduplicates NaClSysSysconf implementation

BUG= none
TEST= small_tests
R=mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=12136

[Mark Seaborn, 2013-09-09 21:39:37]

https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/n...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/n...
src/trusted/service_runtime/nacl_syscall_common.c:3441: if (0 ==
nap->sc_nprocessors_onln) {
Double-checking a field like this will still be reported as a data race by
race-checking tools.

Why do you want to cache the result of GetSystemInfo()?  Do you believe it is
slow?

You're still doing >1 thing in one change.  I'd prefer if you left out this
caching change.

[Petr Hosek, 2013-09-09 21:46:54]

https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/n...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/n...
src/trusted/service_runtime/nacl_syscall_common.c:3441: if (0 ==
nap->sc_nprocessors_onln) {
On 2013/09/09 21:39:38, Mark Seaborn wrote:
> Double-checking a field like this will still be reported as a data race by
> race-checking tools.
> 
> Why do you want to cache the result of GetSystemInfo()?  Do you believe it is
> slow?
> 
> You're still doing >1 thing in one change.  I'd prefer if you left out this
> caching change.

Because the original implementation does the some, just uses an extra static
variable instead of the existing field; I have reverted this change if you
prefer the original way.

[Mark Seaborn, 2013-09-09 21:50:38]

On 9 September 2013 14:46, <phosek@chromium.org> wrote:

>
> https://codereview.chromium.**org/23710020/diff/1/src/**
>
trusted/service_runtime/nacl_**syscall_common.c<https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/nacl_syscall_common.c>
> File src/trusted/service_runtime/**nacl_syscall_common.c (right):
>
> https://codereview.chromium.**org/23710020/diff/1/src/**
>
trusted/service_runtime/nacl_**syscall_common.c#newcode3441<https://codereview.chromium.org/23710020/diff/1/src/trusted/service_runtime/nacl_syscall_common.c#newcode3441>
> src/trusted/service_runtime/**nacl_syscall_common.c:3441: if (0 ==
> nap->sc_nprocessors_onln) {
> On 2013/09/09 21:39:38, Mark Seaborn wrote:
>
>> Double-checking a field like this will still be reported as a data race by
>>
> race-checking tools.
>>
>
>  Why do you want to cache the result of GetSystemInfo()?  Do you believe
>> it is
>>
> slow?
>>
>
>  You're still doing >1 thing in one change.  I'd prefer if you left out
>> this
>>
> caching change.
>>
>
> Because the original implementation does the some, just uses an extra
> static variable instead of the existing field
>

Oh, I didn't notice that.  FWIW, it's worth commenting that sort of thing
in the commit message.

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/groups/opt_out.

[Mark Seaborn, 2013-09-09 21:56:40]

LGTM, thanks

[Petr Hosek, 2013-09-09 23:22:46]

Committed patchset #2 manually as r12136 (presubmit successful).