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

[Pádraig Brady]

On 12/08/10 00:57, Paul Eggert wrote:
> 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.

Fair enough.

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

I agree that the separate --debug code isn't too complicated.
Hopefully we'll be able to keep everything in sync.

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

Let's cross that bridge when we come to it.

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

I think I thought it would be hard
to distinguish temp fds from the -o fd,
but didn't look too hard TBH.

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

Well I can't see a need for mixing the 2 options,
and one could seriously nobble a file in doing so.
I nearly did when testing.

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

A summary of that would be a good comment :)

cheers,
Pádraig.