Re: [coreutils] [PATCH] sort: tune and refactor --debug code, and fix minor underlining bug

[Paul Eggert]

On 08/12/10 01:06, Pádraig Brady wrote:
> The disadvantage of separating the debugging code from the
> actual sorting code is that one now has to maintain the
> extent matching in 2 places, which means we're less sure that
> the debug output matches what's actually being done.

True, but there is a performance advantage of hoisting the debugging
code out of the inner core of comparison.  And if my next few ideas
work out this advantage will become much greater.

Furthermore, separating the debugging code from the actual sorting
code made the code simpler (at least by the trivial measure of lines
of code).  This was a bit counterintuitive for me, but it appears that
the fiddly bits of code to support debugging in the integrated
version were in some sense more complicated than simply cloning the
relevant parts of the code and doing it twice, once for debugging
and once for comparing.

> Also doing within compare(), would have allowed to
> show extents actually used in the comparison of arbitrary lines,
> which could have been enabled with --debug=verbose in future,
> or temporarily to aid development.

That would be a better justification for putting it into the
comparison code.  (The previously-existing code, which enabled
debugging only to compare a line against itself, was pretty weird.)

Perhaps this could be done without adversely affecting performance,
by using inlining to specialize the comparison code for the case
where debugging is disabled.

> I also thought about outputting the total number of comparisons
> it debug mode (though that should be easy to add in in any case I think).

Yes, I think so.

>> -  /* Disable this combination so that users are less likely
>> -     to inadvertantly update a file with debugging enabled.
>> -     Also it simplifies the code for handling temp files.  */
>> -  if (debug && outfile)
>> -    error (SORT_FAILURE, 0, _("options -o and --debug are incompatible"));
>> -
>>    if (debug)
>>      {
>> +      if (checkonly || outfile)
>> +        {
>> +          static char opts[] = "X --debug";
>> +          opts[0] = (checkonly ? checkonly : 'o');
>> +          incompatible_options (opts);
>> +        }
>> +
> 
> I wouldn't have removed the comment, as Eric was wondering
> why -o and --debug were incompatible.

As was I.  I think the comment was wrong in both counts.  I don't see
how it simplified the code for handling temp files.  And I don't see
why it's helpful to nanny the user.  I preserved the incompatibility
betwee -o and --debug only because I didn't want to change too many
things in one edit.  I'd favor removing that incompatibility.

> Also I'm wondering now why --check and --debug are incompatible?

Because --check outputs to stderr, and --debug outputs to stdout.
Before the change, --debug silently overrode --check in this respect,
which was undocumented and confusing (POSIX says that the check option
"shall not" output to stdout).  I thought it better to be honest and
say that the two options don't work together.  I expect that the very
few people who use --check, and who need to debug, can debug without
--check and then use --check once they understand the problem and do
not need to debug any more.