[m-rev.] diff: update prog_io_error.m

[Julien Fischer]

Hi Zoltan,

On Mon, 3 Nov 2014, Zoltan Somogyi wrote:

> Record each kind of error when reading in files.
> 
> compiler/prog_io_error.m:
>     Replace the enum that used to record only whether we found no errors,
>     some nonfatal errors or some fatal errors, with a type that records
>     just what kinds of errors we found. Document each kind of error.
> 
> compiler/prog_io.m:
>     Record any errors using the new type. In some cases, we used to
>     forget fatal errors after finding nonfatal ones; now we don't.
> 
> compiler/red_modules.m:

s/red_modules/read_modules/

>     Do not generate error messages about not being able to open a file
>     if we *could* open the file, but found other fatal errors inside it.
>
>     Factor out some common code.
> 
> compiler/intermod.m:
>     Ignore the error of not being able to open .opt files, but do not
>     ignore any other errors inside them. This fixes a long-standing piece
>     of strange-looking code.
> 
> compiler/module_imports.m:
>     Do not ignore old fatal errors if reading a new interface file
>     gets some new nonfatal errors. This fixes a long-standing XXX.
> 
> compiler/write_module_interface_files.m:
>     Do not ignore fatal errors when considering whether to write out interface
>     files. This fixes a long-standing XXX.
> 
> compiler/modules.m:
>     Generate better error messages for fatal errors inside modules
>     other than not being able to open the module.
> 
> compiler/deps_map.m:
> compiler/make.module_dep_file.m:
> compiler/mercury_compile.m:
> compiler/module_deps_graph.m:
> compiler/recompilation.check.m:
> compiler/trans_opt.m:
> compiler/write_deps_file.m:
>     Conform to the change in prog_io_error.m.

...

> diff --git a/compiler/intermod.m b/compiler/intermod.m
> index 88db87f..1f6df65 100644
> --- a/compiler/intermod.m
> +++ b/compiler/intermod.m

...

> @@ -2552,28 +2552,24 @@ read_optimization_interfaces(Globals, Transitive, ModuleName,
>          !Items, !Specs, !Error, !IO).
>
>  update_error_status(_Globals, FileType, FileName,
> -        ModuleSpecs, !Specs, ModuleError, !Error) :-
> -    (
> -        ModuleError = no_module_errors
> +        ModuleSpecs, !Specs, ModuleErrors, !Error) :-
> +    ( if set.is_empty(ModuleErrors) then
>          % OptSpecs contains no errors. I (zs) don't know whether it could
>          % contain any warnings or informational messages, but if it could,
>          % we should add those error_specs to !Specs. Not doing so preserves
>          % old behavior.
> -    ;
> -        ModuleError = some_module_errors,
> -        !:Specs = ModuleSpecs ++ !.Specs,
> -        !:Error = yes
> -    ;
> -        ModuleError = fatal_module_errors,
> -        % We get here if we couldn't find and/or open the file.
> +        true
> +    else if set.contains(ModuleErrors, rme_could_not_open_file) then
> +        % % We get here if we couldn't find and/or open the file.

Delete the extra '%' there.

>          % ModuleSpecs will already contain an error_severity error_spec
>          % about that, with more details than the message we generate below,
>          % but the test case hard_coded/intermod_unused_args insists on
>          % there being no error, only a warning, and on the text below.
>          % That is why we do not add ModuleSpecs to !Specs here.
>          %
> -        % I (zs) don't know whether adding a version of ModuleSpecs (possibly
> -        % with downgraded severity) to !Specs would be a better idea.
> +        % I (zs) don't know whether we should add a version of ModuleSpecs
> +        % with downgraded severity to !Specs instead of the Spec we generate
> +        % below.
>          (
>              FileType = opt_file,
>              WarningOption = warn_missing_opt_files

...

> diff --git a/compiler/modules.m b/compiler/modules.m
> index 54e3b9a..ebe2a10 100644
> --- a/compiler/modules.m
> +++ b/compiler/modules.m

...

> 
> @@ -1470,17 +1467,19 @@ generate_dependencies(Globals, Mode, Search, ModuleName, DepsMap0, !IO) :-
>
>      map.lookup(DepsMap, ModuleName, ModuleDep),
>      ModuleDep = deps(_, ModuleImports),
> -    Error = ModuleImports ^ mai_error,
> -    (
> -        Error = fatal_module_errors,
> +    Errors = ModuleImports ^ mai_errors,
> +    set.intersect(Errors, fatal_read_module_errors, FatalErrors),
> +    ( if set.non_empty(FatalErrors) then
>          ModuleString = sym_name_to_string(ModuleName),
> -        string.append_list(["can't read source file for module `",
> -            ModuleString, "'."], Message),
> -        report_error(Message, !IO)
> -    ;
> -        ( Error = no_module_errors
> -        ; Error = some_module_errors
> +        ( if set.contains(FatalErrors, rme_could_not_open_file) then
> +            string.append_list(["can't read source file for module `",
> +                ModuleString, "'."], Message)

I suggest: s/can't/cannot/ (and below).


> +        else
> +            string.append_list(["can't parse source file for module `",
> +                ModuleString, "'."], Message)
>          ),
> +        report_error(Message, !IO)
> +    else
>          (
>              Mode = output_d_file_only
>          ;

...

One more comment: throughout this diff you alternate between using
set.empty/0 and set.is_empty/0.  I suggest sticking to the later.  (One
issue with this however is that the standard library only provides
set.non_empty/1, so I suggest adding the synonym set.is_non_empty/1 and
using that as well.)

The diff looks fine otherwise.

Cheers,
Julien.