PLPA Users' Mailing List Archives

Subject: Re: [PLPA users] Patch to remove verbose if-else
From: Jeff Squyres (jsquyres_at_[hidden])
Date: 2008-03-13 17:50:05

• Next message: Bert Wesarg: "[PLPA users] missing clear_cache() in error path"
• Previous message: Brian Curtis: "Re: [PLPA users] Patch to remove verbose if-else"
• In reply to: Brian Curtis: "Re: [PLPA users] Patch to remove verbose if-else"

True, but:

1. Any decent optimizer should be able to see the original code as
equivalent and also optimize it because it's so simple.

2. Even if I'm wrong about #1, are we really caring about performance
of a single operator after returning from a[n expensive] system call?
That seems like overkill, IMHO.

My $0.02 is that optimization is good, but clarity is better where
optimization doesn't matter.

On Mar 13, 2008, at 4:56 PM, Brian Curtis wrote:

> Actually, that is not entirely accurate. There is the possibility for
> compiler optimization based on the ternary operator.
>
>
> Brian
>
> Jeff Squyres wrote:
>> I've always been a verbose guy :-), but I don't really care too much
>> in this situation (the patch is style, not function). I'll apply
>> Paul's suggestion.
>>
>> Thanks guys.
>>
>>
>> On Mar 12, 2008, at 10:42 PM, Paul H. Hargrove wrote:
>>
>>
>>> I find the resulting line too be hard to read, and prone to
>>> misparsing
>>> by a human. Mixing assignment and conditionals in the same
>>> statement
>>> has always been something I've tried to avoid due to the possibility
>>> of
>>> confusion.
>>> I suggest keeping the "ret = syscall(...)" lines unchanged, and only
>>> replacing the if/else constructs with
>>> return (ret >= 0) ? 0 : ret;
>>> or
>>> return (ret < 0) ? ret : 0;
>>> Where, to me, the second option stresses that negative "ret" is
>>> preserved, while the first stresses that positive "ret" are
>>> "squashed"
>>> to zero. Not sure which one wants to emphasize (or even that anyone
>>> else reads these as I do.).
>>> Other than that, I agree w/ Brian that the current if/else is too
>>> much.
>>>
>>> -Paul
>>>
>>> Brian Curtis wrote:
>>>
>>>> Below is a small patch to plpa_dispatch.c to avoid verbose if-else
>>>> statements.
>>>>
>>>>
>>>> Brian
>>>>
>>>>
>>>> --- plpa_dispatch.c 2008-03-11 11:59:23.000000000 -0400
>>>> +++ plpa_dispatch.c 2008-03-12 23:05:56.000000000 -0400
>>>> @@ -110,17 +110,11 @@ int PLPA_NAME(sched_setaffinity)(pid_t p
>>>> memcpy(&tmp, cpuset, cpusetsize);
>>>> }
>>>>
>>>> - /* Now do the syscall */
>>>> - ret = syscall(__NR_sched_setaffinity, pid, PLPA_NAME(len),
>>>> &tmp);
>>>> -
>>>> - /* Return 0 upon success. According to
>>>> + /* Now do the syscall:
>>>> + Return 0 upon success. According to
>>>> http://www.open-mpi.org/community/lists/plpa-users/
>>>> 2006/02/0016.php,
>>>> all the kernel implementations return >= 0 upon
>>>> success. */
>>>> - if (ret >= 0) {
>>>> - return 0;
>>>> - } else {
>>>> - return ret;
>>>> - }
>>>> + return (ret = syscall(__NR_sched_setaffinity, pid,
>>>> PLPA_NAME
>>>> (len), &tmp)) >= 0 ? 0 : ret;
>>>> break;
>>>>
>>>> case PLPA_NAME_CAPS(PROBE_NOT_SUPPORTED):
>>>> @@ -184,17 +178,11 @@ int PLPA_NAME(sched_getaffinity)(pid_t p
>>>> memset(cpuset, 0, cpusetsize);
>>>> }
>>>>
>>>> - /* Now do the syscall */
>>>> - ret = syscall(__NR_sched_getaffinity, pid, PLPA_NAME(len),
>>>> cpuset);
>>>> -
>>>> - /* Return 0 upon success. According to
>>>> + /* Now do the syscall:
>>>> + Return 0 upon success. According to
>>>> http://www.open-mpi.org/community/lists/plpa-users/
>>>> 2006/02/0016.php,
>>>> all the kernel implementations return >= 0 upon
>>>> success. */
>>>> - if (ret >= 0) {
>>>> - return 0;
>>>> - } else {
>>>> - return ret;
>>>> - }
>>>> + return (ret = syscall(__NR_sched_getaffinity, pid,
>>>> PLPA_NAME
>>>> (len), cpuset)) >= 0 ? 0 : ret;
>>>> break;
>>>>
>>>> case PLPA_NAME_CAPS(PROBE_NOT_SUPPORTED):
>>>>
>>>> _______________________________________________
>>>> plpa-users mailing list
>>>> plpa-users_at_[hidden]
>>>> http://www.open-mpi.org/mailman/listinfo.cgi/plpa-users
>>>>
>>>>
>>> --
>>> Paul H. Hargrove PHHargrove_at_[hidden]
>>> Future Technologies Group
>>> HPC Research Department Tel: +1-510-495-2352
>>> Lawrence Berkeley National Laboratory Fax: +1-510-486-6900
>>>
>>>
>>> _______________________________________________
>>> plpa-users mailing list
>>> plpa-users_at_[hidden]
>>> http://www.open-mpi.org/mailman/listinfo.cgi/plpa-users
>>>
>>
>>
>>
> _______________________________________________
> plpa-users mailing list
> plpa-users_at_[hidden]
> http://www.open-mpi.org/mailman/listinfo.cgi/plpa-users

-- 
Jeff Squyres
Cisco Systems

• Next message: Bert Wesarg: "[PLPA users] missing clear_cache() in error path"
• Previous message: Brian Curtis: "Re: [PLPA users] Patch to remove verbose if-else"
• In reply to: Brian Curtis: "Re: [PLPA users] Patch to remove verbose if-else"