incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [patch] exec utility windows port
Date Thu, 10 Aug 2006 00:26:53 GMT
Andrew Black wrote:
> Greetings all.
> 
> Attached is a patch altering source for the the exec utility so that it 
> should build on windows systems.  This patch does not alter the solution 
> generation infrastructure to actually build the utility, but instead 
> changes the source of the utility to make it compatible with windows.

One comment and request: I like the approach taken in exec.cpp
where you abstract the Windows code into new platform-specific
implementations of the existing functions. I would like to take
the same approach in the rest of the sources as well so as to
reduce the amount of platform #ifdefs in executable code. We
need to make the utility easily portable to other non-POSIX,
non-Windows environments (such as z/OS) and the best way to
do that is to abstract out all platform-dependent code into
the smallest possible functions without "polluting" the main
program logic with platform-specific details.

[...]
> Index: cmdopt.cpp
> ===================================================================
> --- cmdopt.cpp	(revision 430088)
> +++ cmdopt.cpp	(working copy)
> @@ -304,7 +308,11 @@
>                  if (optarg && *optarg) {
>                      const long nsec = strtol (optarg, &end, 10);
>                      if ('\0' == *end && 0 <= nsec && !errno) {
> +#if !defined (_WIN32) && !defined (_WIN64)
>                          sleep (nsec);
> +#else
> +                        Sleep (nsec * 1000);
> +#endif

This should be abstracted into a function (say rw_sleep).

>                          break;
>                      }
>                  }
> @@ -330,12 +338,18 @@
>                  if (optarg && *optarg) {
>                      const long signo = get_signo (optarg);
>                      if (0 <= signo) {
> +#if !defined (_WIN32) && !defined (_WIN64)
>                          struct sigaction act;
>                          memset (&act, 0, sizeof act);
>                          act.sa_handler = SIG_IGN;
>                          if (0 > sigaction (signo, &act, 0))
>                              terminate (1, "sigaction(%s, ...) failed: %s\n",
>                                         get_signame (signo), strerror (errno));

As should this. I suggest rw_signal() implemented in terms of
sigaction on POSIX platforms and signal otherwise.

> +#else
> +                        if (SIG_ERR == signal (signo, SIG_IGN))
> +                            terminate (1, "sigaction(%s, ...) failed: %s\n",
> +                                       get_signame (signo), strerror (errno));
> +#endif   /* _WIN{32,64} */
>                          break;
>                      }
>                  }
> @@ -429,7 +443,11 @@
>          }
>          in_token = 1;
>          switch (*pos) {
> +#if !defined (_WIN32) && !defined (_WIN64)
>          case '\\':
> +#else
> +        case '^':
> +#endif   /* _WIN{32,64} */

Hmm. Why can't we use backslash as the escape character on Windows?

>              in_escape = 1;
>              break;
>          case '"':
> Index: output.cpp
> ===================================================================
> --- output.cpp	(revision 430088)
> +++ output.cpp	(working copy)
> @@ -233,8 +233,12 @@
>  {
>      struct stat file_info;
>      const size_t root_len = strlen (in_root);
> -    char* const ref_name = (char*)RW_MALLOC (root_len 
> -                                             + strlen (target_name) + 19);
> +#if !defined (_WIN32) && !defined (_WIN64)
> +    const size_t cmp_len = strlen (target_name);
> +#else
> +    const size_t cmp_len = strlen (target_name) - 4; /* Remove trailing .exe */

Same thing here: something like

     const size_t cmp_len =
         strlen (target_name) - strlen (exe_suffix);

where exe_suffix is a global constant defined to the platform
specific suffix for executable files.

> +#endif   /* _WIN{32,64} */
> +    char* const ref_name = (char*)RW_MALLOC (root_len + cmp_len + 19);
>      FILE* reference;
>  
>      assert (0 != in_root);
> @@ -244,8 +248,13 @@
>  
>      /* Try in_root/manual/out/target_name.out */
>      memcpy (ref_name, in_root, root_len+1);
> +#if !defined (_WIN32) && !defined (_WIN64)
>      strcat (ref_name, "/manual/out/");
>      strcat (ref_name, target_name);
> +#else
> +    strcat (ref_name, "\\manual\\out\\");
> +    strncat (ref_name, target_name, cmp_len);

This should be abstracted out as well. Windows uses the backslash
character, UNIX forward slash, and z/OS the dot as the directory
separator. We need to make it easy to support z/OS (I'm not sure
how extensions like .out are handled...)

If we do a lot of path manipulation in the program we can add
something like rw_path_append(char* path, const char* dir). If
this is the only place where we do path manipulation, I'd go
simply with strcat (strcat (ref_name, path_sep), cmp_len),
and define the global constant path_sep to "/" on UNIX and
to "\\" on Windoze (and "." on z/OS). You might want to check
with Scott Zhong on the z/OS file name specifics.

> +#endif   /* _WIN{32,64} */
>      strcat (ref_name, ".out");
>  
>      if (0 > stat (ref_name, &file_info)) {
[...]
> Index: runall.cpp
> ===================================================================
> --- runall.cpp	(revision 430088)
> +++ runall.cpp	(working copy)
[...]
> @@ -211,6 +223,7 @@
>          const size_t path_len = strlen (target);
>          char* tmp_name;
>  
> +#if 0

Did you really mean to disable this block? (If yes, please add
a comment explaining why.)

[...]
> +#else
> +        /* Or the target\target.obj file on windows systems*/

We should discuss with Anton if we need to have this layout on
Windows instead of the UNIX layout (where all the files are in
the same directory). It would be nice to be consistent.

Martin

Mime
View raw message