[OMPI devel] replace 'atoi' with 'strtol'
Hi,
I want to add a patch to opal mca.
This patch replaces an 'atoi' call with a 'strtol' call.
If it's O.K with everyone I'll submit this patch by the end of the week.
Index: opal/mca/base/mca_base_param.c
===
--- opal/mca/base/mca_base_param.c (revision 14391)
+++ opal/mca/base/mca_base_param.c (working copy)
@@ -1673,7 +1673,7 @@
if (NULL != param->mbp_env_var_name &&
NULL != (env = getenv(param->mbp_env_var_name))) {
if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
- storage->intval = atoi(env);
+ storage->intval = (int)strtol(env,(char**)NULL,0);
} else if (MCA_BASE_PARAM_TYPE_STRING == param->mbp_type) {
storage->stringval = strdup(env);
}
@@ -1714,7 +1714,7 @@
if (0 == strcmp(fv->mbpfv_param, param->mbp_full_name)) {
if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
if (NULL != fv->mbpfv_value) {
-param->mbp_file_value.intval =
atoi(fv->mbpfv_value);
+param->mbp_file_value.intval =
(int)strtol(fv->mbpfv_value,(char**)NULL,0);
} else {
param->mbp_file_value.intval = 0;
}
Thanks.
Sharon.
Re: [OMPI devel] replace 'atoi' with 'strtol'
On Apr 18, 2007, at 7:55 AM, Sharon Melamed wrote:
I want to add a patch to opal mca.
This patch replaces an ‘atoi’ call with a ‘strtol’ call.
If it’s O.K with everyone I’ll submit this patch by the end of the
week.
With the (int) cast, I'm ok with it now. :-)
The man pages on Linux, OSX, and Solaris all describe similar
functionality, so I think you should be ok.
Index: opal/mca/base/mca_base_param.c
===
--- opal/mca/base/mca_base_param.c (revision 14391)
+++ opal/mca/base/mca_base_param.c (working copy)
@@ -1673,7 +1673,7 @@
if (NULL != param->mbp_env_var_name &&
NULL != (env = getenv(param->mbp_env_var_name))) {
if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
- storage->intval = atoi(env);
+ storage->intval = (int)strtol(env,(char**)NULL,0);
} else if (MCA_BASE_PARAM_TYPE_STRING == param->mbp_type) {
storage->stringval = strdup(env);
}
@@ -1714,7 +1714,7 @@
if (0 == strcmp(fv->mbpfv_param, param->mbp_full_name)) {
if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
if (NULL != fv->mbpfv_value) {
-param->mbp_file_value.intval = atoi(fv-
>mbpfv_value);
+param->mbp_file_value.intval = (int)strtol(fv-
>mbpfv_value,(char**)NULL,0);
} else {
param->mbp_file_value.intval = 0;
}
Thanks.
Sharon.
___
devel mailing list
de...@open-mpi.org
http://www.open-mpi.org/mailman/listinfo.cgi/devel
--
Jeff Squyres
Cisco Systems
Re: [OMPI devel] replace 'atoi' with 'strtol'
The patch is so that you can pass in hex in addition to decimal, right? I
think that makes sense. But since we're switching to strtol, it might
also make sense to add some error detection while we're at it. Not a huge
deal, but it would be nice :).
Brian
> Hi,
>
> I want to add a patch to opal mca.
>
> This patch replaces an 'atoi' call with a 'strtol' call.
>
> If it's O.K with everyone I'll submit this patch by the end of the week.
>
>
>
> Index: opal/mca/base/mca_base_param.c
>
> ===
>
> --- opal/mca/base/mca_base_param.c (revision 14391)
>
> +++ opal/mca/base/mca_base_param.c (working copy)
>
> @@ -1673,7 +1673,7 @@
>
>if (NULL != param->mbp_env_var_name &&
>
>NULL != (env = getenv(param->mbp_env_var_name))) {
>
> if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
>
> - storage->intval = atoi(env);
>
> + storage->intval = (int)strtol(env,(char**)NULL,0);
>
> } else if (MCA_BASE_PARAM_TYPE_STRING == param->mbp_type) {
>
>storage->stringval = strdup(env);
>
> }
>
> @@ -1714,7 +1714,7 @@
>
> if (0 == strcmp(fv->mbpfv_param, param->mbp_full_name)) {
>
> if (MCA_BASE_PARAM_TYPE_INT == param->mbp_type) {
>
> if (NULL != fv->mbpfv_value) {
>
> -param->mbp_file_value.intval =
> atoi(fv->mbpfv_value);
>
> +param->mbp_file_value.intval =
> (int)strtol(fv->mbpfv_value,(char**)NULL,0);
>
> } else {
>
> param->mbp_file_value.intval = 0;
>
> }
>
>
>
> Thanks.
>
>
>
> Sharon.
>
> ___
> devel mailing list
> de...@open-mpi.org
> http://www.open-mpi.org/mailman/listinfo.cgi/devel
Re: [OMPI devel] replace 'atoi' with 'strtol'
> With the (int) cast, I'm ok with it now. :-) What's the point of the cast to int? - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
Because the target variable is an (int). Plus, the man pages on OSX and Linux both say that atoi() is the exact equivalent of (int)strtol(nptr, (char **)NULL, 10); and that atoi() is deprecated (which I didn't know). On Apr 18, 2007, at 11:32 AM, Roland Dreier wrote: With the (int) cast, I'm ok with it now. :-) What's the point of the cast to int? - R. ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel -- Jeff Squyres Cisco Systems
Re: [OMPI devel] replace 'atoi' with 'strtol'
And the man page (on OS X) has this prototype for strol(): long strtol(const char * restrict nptr, char ** restrict endptr, int base); I.e., it returns a long. Although some compilers might do the right thing, conversions should be explicitly shown. On Apr 18, 2007, at 11:38 AM, Jeff Squyres wrote: Because the target variable is an (int). Plus, the man pages on OSX and Linux both say that atoi() is the exact equivalent of (int)strtol(nptr, (char **)NULL, 10); and that atoi() is deprecated (which I didn't know). On Apr 18, 2007, at 11:32 AM, Roland Dreier wrote: With the (int) cast, I'm ok with it now. :-) What's the point of the cast to int? - R. ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel -- Jeff Squyres Cisco Systems ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel
Re: [OMPI devel] replace 'atoi' with 'strtol'
> Because the target variable is an (int). If I were writing the code, I would leave the cast out. By assigning the value to an int variable, you get the same effect anyway, so the cast is redundant. And if you ever change the variable to a long, now you have to remember to delete the cast too. So I don't see any upside to having the cast. But it's just a minor style issue...
Re: [OMPI devel] replace 'atoi' with 'strtol'
> I.e., it returns a long. Although some compilers might do the right > thing, conversions should be explicitly shown. Isn't the behavior guaranteed by the C standard? And I can't even imagine a way for a compiler to get intvar = strtol(foo); wrong, and it seems even more implausible that such a bug would be cured just by adding a cast to int. Maybe you could get the same effect by leaving the cast out and wearing a magnetized titanium bracelet while writing the code? - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
It's not only a style issue. There are at least 2 valid reasons to keep the cast explicit. 1. If [somehow] something goes wrong it's a lot simpler to bash a fellow developer than send a request for support to one of the compiler development team. 2. Some compilers don't like the implicit cast. Usually they cope with the side effects but when the int and long don't have the same length (which is usually the case on 64 bits architectures) they generate an error. So, either we add the explicit cast from the beginning or I will have to add it next time I compile on Windows ... george. On Apr 18, 2007, at 11:54 AM, Roland Dreier wrote: Because the target variable is an (int). If I were writing the code, I would leave the cast out. By assigning the value to an int variable, you get the same effect anyway, so the cast is redundant. And if you ever change the variable to a long, now you have to remember to delete the cast too. So I don't see any upside to having the cast. But it's just a minor style issue... ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel smime.p7s Description: S/MIME cryptographic signature
Re: [OMPI devel] replace 'atoi' with 'strtol'
> > Because the target variable is an (int). > > If I were writing the code, I would leave the cast out. By assigning > the value to an int variable, you get the same effect anyway, so the > cast is redundant. And if you ever change the variable to a long, now > you have to remember to delete the cast too. So I don't see any > upside to having the cast. > > But it's just a minor style issue... I agree 100% with Roland on this one. There's a reason that compilers don't complain about this particular cast. Casting from integer type to integer type just isn't a big deal in my book. Of course,I generally try to avoid casts at all costs, since they tend to cover real issues (see all the evil casts of long* to int* that have screwed us continually with 64 bit big endian machines. But I don't care enough to argue the point :). Brian
Re: [OMPI devel] replace 'atoi' with 'strtol'
Roland Dreier wrote: > Because the target variable is an (int). If I were writing the code, I would leave the cast out. By assigning the value to an int variable, you get the same effect anyway, so the cast is redundant. And if you ever change the variable to a long, now you have to remember to delete the cast too. So I don't see any upside to having the cast. But it's just a minor style issue... Some compilers can warn about implicit narrowing conversions, such as one gets without the cast here when sizeof(long)>sizeof(int), though none I know of due so by default. Enabling such warnings can be a good way to look for 32 to 64-bit porting assumptions. However, if one has too many false alarms, such as from intval=strtol(), then the real problems get lost in the noise. As far as I am concerned, avoiding such warnings are the only motivation for including the cast here. This may or may not be sufficient motivation to include it, depending on the developer. -Paul P.S. IIRC the '-Wcheck' option to the Intel compilers is one way to get warnings about implicit narrowing conversions, plus many other "legal but potentially non-portable" code idioms - well beyond the scope of the gcc '-Wall' option. -- Paul H. Hargrove phhargr...@lbl.gov Future Technologies Group HPC Research Department Tel: +1-510-495-2352 Lawrence Berkeley National Laboratory Fax: +1-510-486-6900
Re: [OMPI devel] replace 'atoi' with 'strtol'
> So, either we add the explicit cast from the beginning or I will have > to add it next time I compile on Windows ... I thought the problem with Windows was that long was always 32 bits (the same size as int) even on 64-bit platforms? - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
That's right, long and int have the same size on Windows 32 and 64 bits (always 32 bits). However, they are considered as being different types (!!!). The main problem came from the fact that if we want to use our modular approach on Windows (DLL loaded at runtime) we have to compile in C++ mode. The C++ compiler consider the types int and long as being different (even if they have the same number of bytes). No implicit cast is allowed by the VC compiler when in C++ mode. Therefore, we have to force an explicit cast everywhere. george. On Apr 18, 2007, at 1:04 PM, Roland Dreier wrote: So, either we add the explicit cast from the beginning or I will have to add it next time I compile on Windows ... I thought the problem with Windows was that long was always 32 bits (the same size as int) even on 64-bit platforms? - R. ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel smime.p7s Description: S/MIME cryptographic signature
Re: [OMPI devel] replace 'atoi' with 'strtol'
On Wed, Apr 18, 2007 at 01:16:54PM -0400, George Bosilca wrote: > That's right, long and int have the same size on Windows 32 and 64 > bits (always 32 bits). However, they are considered as being > different types (!!!). How about (u)int32_t? When I was an Ada programmer, subtypes with the approriate range were always encouraged (i.e.: define the semantical range and let the compiler/runtime library warn you on range violations (the well-known "CONSTRAINT_ERROR")) Adr"int consired harmful"ian -- Cluster and Metacomputing Working Group Friedrich-Schiller-Universität Jena, Germany private: http://adi.thur.de
Re: [OMPI devel] replace 'atoi' with 'strtol'
> The main problem came from the fact that if we want to use our > modular approach on Windows (DLL loaded at runtime) we have to > compile in C++ mode. The C++ compiler consider the types int and long > as being different (even if they have the same number of bytes). No > implicit cast is allowed by the VC compiler when in C++ mode. > Therefore, we have to force an explicit cast everywhere. I see... so the right way to right this is really: #ifdef __cplusplus #define ompi_cast_to_int(x) static_cast(x) #else #define ompi_cast_to_int(x) (x) #endif intval = ompi_cast_to_int(stroul(foo)); Right? ;) - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
> How about (u)int32_t? When I was an Ada programmer, subtypes with the > approriate range were always encouraged (i.e.: define the semantical > range and let the compiler/runtime library warn you on range > violations (the well-known "CONSTRAINT_ERROR")) It's OK to use a type with a fixed size when there is some real reason that you know exactly how many bits you need, but in my opinion it's much better to use plain int whenever possible. Otherwise you end up in a mess when different functions have parameters of different widths for no good reason, and you're also taking away the compiler's choice to use the most efficient size of integer. - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
> I see... so the right way to right this is really: err... "right way to WRITE this" - R.
Re: [OMPI devel] replace 'atoi' with 'strtol'
I really don't want to go that far. I already had to deal with the bool to int conversion in exactly this way. And changing it all over the code, was a mess. As the simple explicit cast is enough to keep happy the VC compiler, I propose we stick with it. It's less intrusive, impose less [unusual] things to the developers, and still give us the benefit of allowing Open MPI to get compiled on Windows. Thanks, george. On Apr 18, 2007, at 4:13 PM, Roland Dreier wrote: The main problem came from the fact that if we want to use our modular approach on Windows (DLL loaded at runtime) we have to compile in C++ mode. The C++ compiler consider the types int and long as being different (even if they have the same number of bytes). No implicit cast is allowed by the VC compiler when in C++ mode. Therefore, we have to force an explicit cast everywhere. I see... so the right way to right this is really: #ifdef __cplusplus #define ompi_cast_to_int(x) static_cast(x) #else #define ompi_cast_to_int(x) (x) #endif intval = ompi_cast_to_int(stroul(foo)); Right? ;) - R. ___ devel mailing list de...@open-mpi.org http://www.open-mpi.org/mailman/listinfo.cgi/devel smime.p7s Description: S/MIME cryptographic signature
