Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

Subject: Re: [OMPI devel] [OMPI svn-full] svn:open-mpi r24665
From: Larry Baker (baker_at_[hidden])
Date: 2011-05-02 14:17:00


Ralph,

What about creating a lookup table of static const values with
comments for readability, and use Tim's code, except for the last
line, which would lookup the value instead of computing it?

I don't know how often this code path is traversed. Seeing only this
snippet of code, I prefer Tim's code because it is clear what the
valid values are for the input argument (no need to scan all the
"case"s, find there is a "default", and deduce what the valid input
values are), it is more efficient in space and time, and, to my eyes,
more readable (I don't have to know what parse_dots() returns). I
suppose a case could also be made that Tim's code is more
maintainable, given the discovery already of a misplaced (though
benign) break and the possibility of a typo in all those calls to
parse_dots().

Just my $.02

Larry Baker
US Geological Survey
650-329-5608
baker_at_[hidden]

On May 1, 2011, at 7:44 AM, Ralph Castain wrote:

> Mostly because I thought it of some value to make the resulting mask
> readable and apparent to someone looking at the code.
>
>
> On Apr 30, 2011, at 8:14 PM, Tim Mattox wrote:
>
>> Why not do this instead of a big switch statement?
>>
>> pval = strtol(msk, NULL, 10);
>> if ((pval > 30) || (pval < 1)) {
>> opal_output(0, "opal_iftupletoaddr: unknown mask");
>> free(addr);
>> return OPAL_ERROR;
>> }
>> *mask = 0xFFFFFFFF << (32 - pval);
>>
>>
>> On Fri, Apr 29, 2011 at 1:56 PM, <rhc_at_[hidden]> wrote:
>>> Author: rhc
>>> Date: 2011-04-29 13:56:15 EDT (Fri, 29 Apr 2011)
>>> New Revision: 24665
>>> URL: https://svn.open-mpi.org/trac/ompi/changeset/24665
>>>
>>> Log:
>>> Cover all the netmask values
>>>
>>> Text files modified:
>>> trunk/opal/util/if.c | 103 ++++++++++++++++++++++++++++++++++++
>>> +--
>>> 1 files changed, 96 insertions(+), 7 deletions(-)
>>>
>>> Modified: trunk/opal/util/if.c
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- trunk/opal/util/if.c (original)
>>> +++ trunk/opal/util/if.c 2011-04-29 13:56:15 EDT (Fri, 29
>>> Apr 2011)
>>> @@ -534,13 +534,102 @@
>>> * much of the addr to use: e.g., /16
>>> */
>>> pval = strtol(msk, NULL, 10);
>>> - if (24 == pval) {
>>> - *mask = 0xFFFFFF00;
>>> - } else if (16 == pval) {
>>> - *mask = 0xFFFF0000;
>>> - } else if (8 == pval) {
>>> - *mask = 0xFF000000;
>>> - } else {
>>> + switch(pval) {
>>> + case 30:
>>> + *mask = parse_dots("255.255.255.252");
>>> + break;
>>> + case 29:
>>> + *mask = parse_dots("255.255.255.248");
>>> + break;
>>> + case 28:
>>> + *mask = parse_dots("255.255.255.240");
>>> + break;
>>> + case 27:
>>> + *mask = parse_dots("255.255.255.224");
>>> + break;
>>> + case 26:
>>> + *mask = parse_dots("255.255.255.192");
>>> + break;
>>> + case 25:
>>> + *mask = parse_dots("255.255.255.128");
>>> + break;
>>> + case 24:
>>> + break;
>>> + *mask = parse_dots("255.255.255.0");
>>> + break;
>>> + case 23:
>>> + *mask = parse_dots("255.255.254.0");
>>> + break;
>>> + case 22:
>>> + *mask = parse_dots("255.255.252.0");
>>> + break;
>>> + case 21:
>>> + *mask = parse_dots("255.255.248.0");
>>> + break;
>>> + case 20:
>>> + *mask = parse_dots("255.255.240.0");
>>> + break;
>>> + case 19:
>>> + *mask = parse_dots("255.255.224.0");
>>> + break;
>>> + case 18:
>>> + *mask = parse_dots("255.255.192.0");
>>> + break;
>>> + case 17:
>>> + *mask = parse_dots("255.255.128.0");
>>> + break;
>>> + case 16:
>>> + *mask = parse_dots("255.255.0.0");
>>> + break;
>>> + case 15:
>>> + *mask = parse_dots("255.254.0.0");
>>> + break;
>>> + case 14:
>>> + *mask = parse_dots("255.252.0.0");
>>> + break;
>>> + case 13:
>>> + *mask = parse_dots("255.248.0.0");
>>> + break;
>>> + case 12:
>>> + *mask = parse_dots("255.240.0.0");
>>> + break;
>>> + case 11:
>>> + *mask = parse_dots("255.224.0.0");
>>> + break;
>>> + case 10:
>>> + *mask = parse_dots("255.192.0.0");
>>> + break;
>>> + case 9:
>>> + *mask = parse_dots("255.128.0.0");
>>> + break;
>>> + case 8:
>>> + *mask = parse_dots("255.0.0.0");
>>> + break;
>>> + case 7:
>>> + *mask = parse_dots("254.0.0.0");
>>> + break;
>>> + case 6:
>>> + *mask = parse_dots("252.0.0.0");
>>> + break;
>>> + case 5:
>>> + *mask = parse_dots("248.0.0.0");
>>> + break;
>>> + case 4:
>>> + *mask = parse_dots("240.0.0.0");
>>> + break;
>>> + case 3:
>>> + *mask = parse_dots("224.0.0.0");
>>> + break;
>>> + case 2:
>>> + *mask = parse_dots("192.0.0.0");
>>> + break;
>>> + case 1:
>>> + *mask = parse_dots("128.0.0.0");
>>> + break;
>>> + case 0:
>>> + *mask = parse_dots("0.0.0.0");
>>> + break;
>>> + default:
>>> opal_output(0, "opal_iftupletoaddr: unknown
>>> mask");
>>> free(addr);
>>> return OPAL_ERROR;
>>> _______________________________________________
>>> svn-full mailing list
>>> svn-full_at_[hidden]
>>> http://www.open-mpi.org/mailman/listinfo.cgi/svn-full
>>>
>>
>>
>>
>> --
>> Tim Mattox, Ph.D. - http://homepage.mac.com/tmattox/
>> timattox_at_[hidden] || tmattox_at_[hidden]
>> I'm a bright... http://www.the-brights.net/
>>
>> _______________________________________________
>> devel mailing list
>> devel_at_[hidden]
>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
>
> _______________________________________________
> devel mailing list
> devel_at_[hidden]
> http://www.open-mpi.org/mailman/listinfo.cgi/devel