[m-rev.] for review: printing error messages in order

[Zoltan Somogyi]

On 04-Aug-2009, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> Why have two places where the same information is stored?

Historical reasons.

> Will this be
> changed in the future?

Hopefully, yes. This is a step towards that.

> > +:- pred maybe_write_out_errors_no_module(bool::in, globals::in,
> > +    list(error_spec)::in, list(error_spec)::out, io::di, io::uo) is det.
> 
> This declaration looks like it is in the interface of this module.  It should
> probably have a documentation string above it.

The implementation has comment directing you to the documentation.

> > +        Verbose = no
> > +    ;
> > +        Verbose = yes,
> > +        % XXX _NumErrors
> > +        write_error_specs(!.Specs, Globals,
> > +            0, _NumWarnings, 0, _NumErrors, !IO),
> > +        !:Specs = []
> > +    ).
> > +
> 
> What is ment by XXX.  My attention is drawn to _NumErrors but I don't
> know why.

We want to keep a record, or at least a count, of all the errors in the
program. Places in the compiler that print out errors but do NOT add
NumErrors to the error count in the HLDS have been marked by this exact
comment for a long time, specifically so that later they can be fixed.
This diff simply replaces places where error messages were being printed
without this recording with new, better ordered places where error messages
are being printed without this recording :-(

> > +    % 
> > +    % XXX Using this mechanism to let make_hlds know this is a bad design.
> > +    OptItems = cord.list(OptItemsCord),
> > +    AddedItems = [make_pseudo_decl(md_opt_imported) | OptItems],
> > +    module_and_imports_add_items(cord.from_list(AddedItems), !Module),
> > +    module_and_imports_add_specs(OptSpecs, !Module),
> 
> Why turn a cord into a list just to cons something to it and then turn it back
> into a cord?  I recommend using the cord module's cons function.

I will do this, though the efficiency gain is totally negligible.

> > -:- pred modecheck_module(module_info::in, module_info::out,
> > -    modes_safe_to_continue::out, list(error_spec)::out) is det.
> > +:- pred modecheck_module(module_info::in,
> > +    {module_info, modes_safe_to_continue, list(error_spec)}::out) is det.
> >  
> >      % Mode-check or unique-mode-check the code of all the predicates
> >      % in a module.
> 
> When reading this without the context of the changelog I don't know why a tuple
> is used here.  Please add a small comment.

Will do.

> > -    % XXX zs: why does this code not check for fatal_module_errors?
> > -    ( Error = some_module_errors ->
> > +    % XXX zs: why is fatal_module_errors with no_module_errors instead of
> > +    % some_module_errors?
> 
> This sentence doesn't read well.  I don't know what you mean.

It doesn't surprise you to learn that the fatal error case is treated
the same way as the no error case?

> > +        % XXX _NumErrors
> 
> As above.  Perhaps there's a comment somewhere that describes all of these.  If
> so these should point me at that comment.

Isn't it obvious that ignoring the number of errors printed is a potential
problem?

> > +        % XXX zs: why is fatal_module_errors with no_module_errors instead of
> > +        % some_module_errors?
> > +        (
> > +            Error = some_module_errors,
> > +            % XXX _NumErrors
> 
> These comments again.  I now know what you mean, you're referring to the cases
> of the switch arms.

Another problem of course is the code semi-duplication that required that code
pattern to be repeated. However, fixing that is a different kettle of fish.

> >                  % XXX _NumErrors
> 
> And here.

There is no point in pointing out the same "problem" over and over again
in a review, unless the spots where the problem occurs are hard to see.

> > --- compiler/state_var.m	28 Jul 2008 08:34:19 -0000	1.27
> > +++ compiler/state_var.m	22 Jul 2009 03:10:39 -0000
> > @@ -353,6 +353,7 @@
> >  
> >  :- import_module char.
> >  :- import_module int.
> > +:- import_module io.
> >  :- import_module pair.
> >  :- import_module string.
> >  :- import_module svmap.
> 
> This is the only change in this module.  Unless this module didn't compile
> without this change then this change is not required.

It does NOT compile without this change. The log message explains why.
The reason is a weakness in the Mercury's handling of nested modules.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------