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

[Paul Bone]

On Tue, Aug 04, 2009 at 10:01:34PM +1000, Zoltan Somogyi wrote:
> On 04-Aug-2009, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > > +        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 :-(

There should be a comment to this effect somewhere so that noone needs to
explain this to anyone else in the near future :-).

> > > +    % 
> > > +    % 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.

I agree that this is negligible, I think it's more straight-forward though
which is probably more valuable.

> > > -:- 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?

It does, definitly.  But I didn't know what that sentance ment when I saw it in
this instance.

> > > +        % 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?

It might be, but I didn't know why you think it's a potential problem.

> > >                  % 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.

Okay, that's fair enough.

> > > --- 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.
> 

Sorry, when reviewing changes in the future I'll put more effort into
cross-checking changes with the description of the changes in the changelog and
similar changes and code in the rest of the diff.

Thanks.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20090805/03b4eb0b/attachment.sig>