[m-rev.] for review: io.make_temp now reterns a result rather than throw exceptions

[Paul Bone]

On Wed, Apr 20, 2016 at 04:50:32PM +1000, Peter Wang wrote:
> > @@ -1274,7 +1276,15 @@ invoke_mkinit(Globals, InitFileStream, Verbosity,
> >                  MkInitOK = no
> >              )
> >          ;
> > -        OpenResult = error(_),
> > +            OpenResult = error(Error),
> > +            io.format(io.stderr_stream, "%s: %s\n",
> > +                [s(TmpFile), s(error_message(Error))], !IO),
> > +            MkInitOK = no
> > +        )
> > +    ;
> > +        TmpFileResult = error(Error),
> > +        io.format(io.stderr_stream, "Could not create temporary file: %s\n",
> > +            [s(error_message(Error))], !IO),
> >          MkInitOK = no
> >      ).
> 
> I think invoke_mkinit should return the error to be presented by its
> callers.

I started trying to make this change.  It created a chain reaction of
refactoring throughout this module,  I got half-way done and stopped.  It's
not worth doing as part of this change, it should be done separately.

> > @@ -1977,8 +1987,12 @@ link_exe_or_shared_lib(Globals, ErrorStream, LinkTargetType, ModuleName,
> >              % and thus overflow the command line, so in this case
> >              % we first create an archive of all of the object files.
> >              RestrictedCommandLine = yes,
> > -            io.make_temp(TmpFile, !IO),
> >              globals.lookup_string_option(Globals, library_extension, LibExt),
> > +            io.get_temp_directory(TempDir, !IO),
> > +            io.make_temp_res(TempDir, "", "." ++ LibExt, TmpFileResult, !IO),
> > +            (
> > +                TmpFileResult = ok(TmpFile),
> > +                % XXX: This is why my /tmp directory is always full of crap.
> >                  TmpArchive = TmpFile ++ LibExt,
> >                  % Only include actual object files in the temporary archive,
> >                  % not other files such as other archives.
> 
> Delete the comment.
> 
> --restricted-command-line is only used on Windows so I doubt that's what
> you're seeing.  It is a bug, though.

Whoops, I should have grepped my diff for XXX.

I also introduced a bug here because I failed to revisit the XXX.  Fixed
now.

> > diff --git a/compiler/write_deps_file.m b/compiler/write_deps_file.m
> > index e661e06..caac87b 100644
> > --- a/compiler/write_deps_file.m
> > +++ b/compiler/write_deps_file.m
> > @@ -159,8 +159,14 @@ write_dependency_file(Globals, ModuleAndImports, AllDeps,
> >      % parallel makes, we first create the file with a temporary name,
> >      % and then rename it to the desired name when we've finished.
> >  
> > -    io.make_temp(dir.dirname(DependencyFileName), "tmp_d",
> > -        TmpDependencyFileName, !IO),
> > +    io.make_temp_res(dir.dirname(DependencyFileName), "tmp_d",
> > +        "", TmpDependencyFileNameRes, !IO),
> > +    (
> > +        TmpDependencyFileNameRes = error(Error),
> > +        Message = "Could not create temporary file: " ++ error_message(Error),
> > +        report_error(Message, !IO)
> > +    ;
> > +        TmpDependencyFileNameRes = ok(TmpDependencyFileName),
> >          maybe_write_string(Verbose, "% Writing auto-dependency file `", !IO),
> >          maybe_write_string(Verbose, DependencyFileName, !IO),
> >          maybe_write_string(Verbose, "'...", !IO),
> 
> [snip!]
> 
> I think you should add a helper predicate (just in the compiler
> directory) that does make_temp_file followed by open_output:
> 
>     :- pred open_temp_output(io.res(pair(string, text_output_stream))::out,
>          io::di, io::uo) is det.
> 
> then this huge predicate does not need another level of indentation.
> 
> Other changes in this diff would benefit from using this predicate as
> well.

It wouldn't help in this case.  In this case:
    + The temp file name is chosen and created.
    + A system command is executed to write to the new file.
    + The file is opened for input.

But it'd still be useful, I'll implement it for both patterns.

> > diff --git a/library/io.m b/library/io.m
> > index 41a3f10..a944075 100644
> > --- a/library/io.m
> > +++ b/library/io.m

> >      % name is different to the name of any existing file. The file will reside
> >      % in the directory specified by Dir and will have a prefix using up to
> > -    % the first 5 characters of Prefix. Name is bound to the name of the
> > -    % file.  It is the responsibility of the caller to delete the file when it
> > -    % is no longer required.
> > -    %
> > -    % Throws an io.error exception if the temporary file could not be created.
> > +    % the first 5 characters of Prefix. If successful, Result returns the name
> > +    % of the file.  It is the responsibility of the caller to delete the file
> > +    % when it is no longer required.
> >      %
> >      % The C# backend has the following limitations:
> >      %   - Dir is ignored.
> >      %   - Prefix is ignored.
> > +    %   - Suffix is ignored.
> > +    %
> > +    % On the Erlang backend Suffix is ignored.
> >      %
> >      % On the Erlang and Java backends, this does not attempt to create the file
> >      % with restrictive permissions (600 on Unix-like systems) and therefore
> >      % should not be used when security is required.
> >      %
> > -:- pred make_temp(string::in, string::in, string::out, io::di, io::uo)
> > -    is det.
> > +:- pred make_temp_res(string::in, string::in, string::in, io.res(string)::out,
> > +    io::di, io::uo) is det.
> 
> The list of exceptions gets longer and longer :(
> 
> Ignoring the Suffix seems worse than ignoring the Dir and Prefix;
> presumably the caller wants a specific file extension?
> If Suffix can't be supported on C#, perhaps we should just drop it.
> It's only used so far in that --restricted-command-line path, which
> could be handled some other way (e.g. renaming the temp file).
> Unfortunately the log message that added the ++ LibExt does not say why
> it was added.

It seems better to improve the C# implementation.  We just haven't done that
yet.


-- 
Paul Bone