Re: regression: filling comments in C++ code (today's CVS)

[Martin Stjernholm]

"Stefan Monnier" <monnier+gnu/address@hidden> wrote:

> > 1.  fill-paragraph is called.
> > 2.  c-fill-paragraph is called by fill-paragraph-function. It does its
> >     comment masking etc. Then it dynamically binds
> >     fill-paragraph-function to nil and calls fill-paragraph again.
> > 3.  fill-paragraph now runs a second time and performs its default
> >     action.
> > 4.  c-fill-paragraph cleans up the masking, restores
> >     fill-paragraph-function and returns.
> > 5.  fill-paragraph returns.
> 
> The step 4 together with the second half of step 2 amount to
> 
>       (let (fill-paragraph-function)
>         (fill-paragraph ...))
/.../

Sorry, I failed to make my point clear. It's not the dynamic binding
of the hook, it's that fill-paragraph doubles as both the wrapper that
calls fill-paragraph-function and as the provider of the default
implementation for it. I argued that it'd be cleaner if it only called
fill-paragraph-function which instead had the default implementation
installed as its default value. (Anyway, as said earlier, the issue is
moot for the compatibility reasons.)

> > > > It'd be nice if fill.el provided a function that only did the pure
> > > > text filling between two positions, preferably also with the option to
> > > 
> > > Isn't that what fill-region-as-paragraph does ?
> > 
> > It has adaptive-fill-mode stuff hardcoded. Although easy to shut down
> > by covering the adaptive-fill-mode variable, it'd be cleaner to have
> > a basic filling function that doesn't have any such "surprises".
> 
> I don't necessarily disagree but I think it's a rather minor issue.

Yes, that particular one is minor since it's been around a long time,
but things like it can be a problem when they are introduced in new
versions. The difference between the user level task and the lower
level utility function (which I'm advocating here) is that the former
is allowed to accumulate more such customizable settings while the
latter should have completely well defined behavior (if it gets more
features they'll be controlled through more arguments instead).

> > And for some reason it skips empty lines at the beginning of the
> > region, something that should be up to the caller to do. It might
> > contain other similar oddities too.
> 
> I'm not sure what you mean by "skips empty lines".

I mean the following near the start of fill-region-as-paragraph:

  ;; Ignore blank lines at beginning of region.
  (skip-chars-forward " \t\n")

To me it seems more natural if this function compacted all whitespace
the same way, even that at the start.

Btw, if it skips \n then it should perhaps skip \r too so that it
works with selective-display.