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

[Stefan Monnier]

> > > Well, it's not that Kyle E Jones just was being stupid when he wrote
> > > filladapt. He can't do it any better way since there's no hook to
> > > replace fill-context-prefix which implements adaptive-fill-mode.
> > 
> > I don't understand.  Why can't filladapt check (and call)
> > fill-paragraph-function before doing anything else ?
> 
> I.e. filladapt still overrides fill-paragraph but it checks
> fill-paragraph-function earlier itself. You're right, it could do
> that.
> 
> CC Mode would still have to use fill-paragraph-function in a fairly
> convoluted way, though:
> 
> 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 ...))

and as a matter of fact, the let binding is already done by fill-paragraph
for you (don't know if filladapt does it as well or not, tho), so there's
really nothing special to do other than "return a non-nil value".

That let-binding is also already done by c-fill-paragraph, by the way.

> That'd be workable, but it's not a very pretty solution.

It's the way it's meant to work.  In practice (barring compatibility
issues with filladapt, XEmacs, and Emacs < 20.7, i.e. things I don't
know about) it simply looks like the following:

    (set (make-local-variable 'fill-paragraph-function) 'foo-fill-para)
and
    (defun foo-fill-para (...)
      ...
      (fill-paragraph ...)
      ...)

where the inner call is typically wrapped in a `let' that rebinds
a few vars such as adaptive-fill-regexp, paragraph-start, ...
or it might directly call fill-region-as-paragraph or do the
filling itself.

> A potential
> problem is if fill-paragraph has some recursion detection (it often
> has, but in the versions I've checked that wouldn't affect this case).

It indeed does and that detection is "set fill-paragraph-function to nil
while calling it" which is exactly what you suggested in steps 2 and 4.

> A cleaner approach would be to make fill-paragraph a function that
> only calls fill-paragraph-function and move the current functionality
> in it to a function that is installed on fill-paragraph-function by
> default. CC Mode could then install a function on
> fill-paragraph-function that chains on to the previous one. (Other
> packages like filladapt would do the same and some effort would be
> necessary in CC Mode to ensure it is installed last, but it's usually
> not very difficult for a major mode to do that.)

I don't see why the current system can't do the exact same thing:
the only difference is that the value corresponding to the default
is nil instead of being fill-paragraph and that instead of

        (funcall next-fun ...)

you have to do

        (let ((fill-paragraph-function next-fun))
          (fill-paragraph ...))

> Anyway, there are compatibility problems with this suggestion, so it's
> probably easier just to live with it.

Indeed.

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

> It's also line oriented if I remember correctly, so it can affect
> parts of the lines that are outside the two positions. A fairly
> peculiar case but still something that CC Mode has to do some trickery
> for so that code close by a comment doesn't get filled too.

I think I've tried to improve this, but I can't remember how far I've
gotten.  I definitely consider it as a bug when f-r-a-p modifies the
buffer outside of the specified region.

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

> > That would help in the "normal" case, so the question is not "would it
> > work with filladapt" but "would it hurt with filladapt".
> > AFAIK it can't hurt, but I don't know enough about filladapt (or about
> > cc-mode's filling) to be sure.
> 
> No, it probably wouldn't. I'll make that change.

Great, thanks,


        Stefan