Re: [bug-diffutils] Diffutils 3.2 v. VMS

[Steven M. Schweda]

From: Paul Eggert <address@hidden>

> >    There are still some minor compiler complaints caused by time_t being
> > unsigned around here:
> 
> That's OK.  The code's correct, so the compiler diagnostics are mistaken,
> so we can ignore them.  Perhaps you can use compiler flags to shut
> them off.

   It's possible, but "-I-" means "informational", so the biggest
problem is appearance at build time.  (My complaint about these compiler
complaints can be taken the same way.)

> > %CC-I-LONGDOUBLENYI, In this declaration, type long double has the same
> >  representation as type double on this platform.
> 
> Again, that's OK.  The code's fine, and works correctly when long double
> is the same as double, so the warnings are not helpful and can be ignored.
> Some POSIXish systems are similar.  They have unsigned time_t and
> sometimes their compilers warn.  We ignore the warnings there too.

   Fine with me.

> So this change:
> 
> -                   if (value[i] == -1)
> +                   if (value[i] == (uintmax_t)-1)
> 
> should not be needed: value[i] is of type uintmax_t, so the
> comparison should be done correctly without the cast.

   While it may not be "needed", my preference is to make the type cast
explicit when treating an obviously signed value as an unsigned value,
and doing so has the added benefit of clearing the compiler complaint.
But, again, the complaint is only "informational", and it's not my code. 
(I think that we've already established that our coding styles differ.)

> As for the other changes, I'd rather not have "#ifdef __VMS"
> in the mainline code.  The mainline code should be as close to
> GNU- or POSIX-compatible as possible.  VMS-specific stuff should
> be in a separate area, so that maintainers of the regular code
> don't need to worry about it.  [...]

   In general, that's ok by me.  (Although I'd've figured that a
properly indented "#ifdef __VMS" - "#endif /* def __VMS */" block would
usually be pretty easy to ignore.)

>   For example, instead of this:
> 
> +#ifdef __VMS
> +# define OPEN_ARG , "ctx = stm"
> +#else /* def __VMS */
> +# define OPEN_ARG
> +#endif /* def __VMS [else] */
> +
>  #if defined LC_MESSAGES && ENABLE_NLS
>  # define hard_locale_LC_MESSAGES hard_locale (LC_MESSAGES)
>  #else
> @@ -296,7 +302,7 @@
>           xfreopen (NULL, "rb", stdin);
>       }
>        else
> -     file_desc[f1] = open (file[f1], O_RDONLY | O_BINARY, 0);
> +     file_desc[f1] = open (file[f1], O_RDONLY | O_BINARY, 0 OPEN_ARG);
> 
> I'd rather that we leave this code alone, and have the wrapper
> for 'open' (in the lib subdirectory) deal with OPEN_ARG.

   Normally I'd agree, but in this case I was trying to fix a specific
problem, and I don't know how many other instances of open() would be
affected/damaged by a global change.  Defining an OPEN (or CMP_OPEN, or
something) macro (somewhere) would allow a line like:
        file_desc[f1] = OPEN (file[f1], O_RDONLY | O_BINARY, 0);
And the VMS definition of that macro could insert the extra argument(s). 
I don't care much, so long as I can get the stuff I need where I need
it, and not get it where I don't know what it'll wreck.

>   And
> instead of this:
> 
> +#ifdef __VMS
> +  /* No "/" required (or desired) on VMS. */
> +  sprintf (tmpl, "%.*s%.*sXXXXXX", (int) dlen, dir, (int) plen, pfx);
> +#else /* def __VMS */
>    sprintf (tmpl, "%.*s/%.*sXXXXXX", (int) dlen, dir, (int) plen, pfx);
> +#endif /* def __VMS [else] */
> 
> I'd rather have something like this:
> 
> #ifndef DIRECTORY_SEPARATOR_STRING
> # define DIRECTORY_SEPARATOR_STRING "/"
> #endif
> 
>    sprintf (tmpl, "%.*s" DIRECTORY_SEPARATOR_STRING "%.*sXXXXXX", (int) dlen, 
> dir, (int) plen, pfx);
> 
> where some other file (dirname.h, perhaps) defines DIRECTORY_SEPARATOR_STRING
> for VMS.

   This change is specific to the use of P_tmpdir (defined in <stdio.h>
around here), so I'd probably use a macro name more like
TMPDIR_something rather than DIRECTORY_something, and I'd probably just
stick its definition near the end of config.h (vms/config.h), but I
don't care much.  (I don't already have a wrapper for <stdio.h> (or a
replacement for <dirname.h>, which VMS lacks), but it (or the other)
would be easy to add.)

   I probably could have hidden this one (src/diff.c) somewhere in the
VMS-specific initialization:
        ignore_file_name_case = vms_case_blind();
but I thought that it would be clearer to have it where all the other
option-related processing was done.  (At least I buried the details in
vmslib/vms.c:vms_case_blind().)  One problem with better hiding
something like this is that it'll be hidden, and sometimes it's nice to
know what's going on without having to search all over to find out about
it.

   The VMS specifics for vfork()-related I/O in src/diff3.c and
src/sdiff.c were so thoroughly interleaved that I didn't try to hide
them.  (I was so amazed that I could get the stuff to work at all, that
I was in no hurry to start rearranging it all, either.  Resurrecting
HAVE_WORKING_VFORK and its friends could allow hiding (or disguising)
some of the VMS specificity, but not much.)

   As usual, I'm not wedded to any particular details, and I'll
(gratefully) take what I can get.

------------------------------------------------------------------------

   Steven M. Schweda               address@hidden
   382 South Warwick Street        (+1) 651-699-9818
   Saint Paul  MN  55105-2547