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

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

[Robert Anderson]

I just updated from CVS today and it appears there has been a
rather serious regression with respect to filling comments in C++
code.

Here's a test case, in cc-mode:

main()
{
    // a[M-q]

comes back with:

fill-comment-paragraph: Search failed: "/\\*+ *\\|//+ *"

Bob

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

[Stefan Monnier]

> I just updated from CVS today and it appears there has been a
> rather serious regression with respect to filling comments in C++
> code.
> 
> Here's a test case, in cc-mode:
> 
> main()
> {
>     // a[M-q]
> 
> comes back with:
> 
> fill-comment-paragraph: Search failed: "/\\*+ *\\|//+ *"

Could you provide the full backtrace (by setting
Options => Enter Debugger on Error) ?


	Stefan

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

[Robert Anderson]

--- Original Message ---
From: "Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu>
To: "Robert Anderson" <RWA@sbcglobal.net>
CC: emacs-devel@gnu.org
Subject: Re: regression: filling comments in C++ code (today's CVS) 

>> I just updated from CVS today and it appears there has been a
>> rather serious regression with respect to filling comments in C++
>> code.
>> 
>> Here's a test case, in cc-mode:
>> 
>> main()
>> {
>>     // a[M-q]
>> 
>> comes back with:
>> 
>> fill-comment-paragraph: Search failed: "/\\*+ *\\|//+ *"
>
>Could you provide the full backtrace (by setting
>Options => Enter Debugger on Error) ?
>
>
>
Stefan

I guess a _15 character_ test case is too much work to cut/paste? :)

Here 'tis:

Debugger entered--Lisp error: (search-failed "/\\*+ *\\|//+ *")
  re-search-forward("/\\*+ *\\|//+ *")
  fill-comment-paragraph(nil)
  fill-paragraph(nil)
  apply(fill-paragraph nil)
  c-mask-comment(t nil fill-paragraph nil)
  c-fill-paragraph(nil)
  call-interactively(c-fill-paragraph)

Thanks,
Bob

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

[Stefan Monnier]

> >> I just updated from CVS today and it appears there has been a
> >> rather serious regression with respect to filling comments in C++
> >> code.
> >> 
> >> Here's a test case, in cc-mode:
> >> 
> >> main()
> >> {
> >>     // a[M-q]
> >> 
> >> comes back with:
> >> 
> >> fill-comment-paragraph: Search failed: "/\\*+ *\\|//+ *"
> >
> >Could you provide the full backtrace (by setting
> >Options => Enter Debugger on Error) ?
> 
> I guess a _15 character_ test case is too much work to cut/paste? :)

The difference is between "a few seconds for each bug reporter"
and "a few seconds * N bug reports for each maintainer".  We don't
have too much maintainer-time in our hands.

> Here 'tis:
> 
> Debugger entered--Lisp error: (search-failed "/\\*+ *\\|//+ *")
>   re-search-forward("/\\*+ *\\|//+ *")
>   fill-comment-paragraph(nil)
>   fill-paragraph(nil)
>   apply(fill-paragraph nil)
>   c-mask-comment(t nil fill-paragraph nil)
>   c-fill-paragraph(nil)
>   call-interactively(c-fill-paragraph)

Why on earth does cc-mode rebind M-q instead of setting
fill-paragraph-function ?

Anyway, it's obviously a problem in my new filling code.
I'll take a look at it, but in the mean time, rebinding
M-q to fill-paragraph (in c-common-mode-hook I guess)
and setting fill-paragraph-function to `c-fill-paragraph'
should hide the problem.


	Stefan



-------------------------------------------------------
This SF.NET email is sponsored by: Thawte.com - A 128-bit supercerts will
allow you to extend the highest allowed 128 bit encryption to all your 
clients even if they use browsers that are limited to 40 bit encryption. 
Get a guide here:http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0030en

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

[Stefan Monnier]

> Debugger entered--Lisp error: (search-failed "/\\*+ *\\|//+ *")
>   re-search-forward("/\\*+ *\\|//+ *")
>   fill-comment-paragraph(nil)
>   fill-paragraph(nil)
>   apply(fill-paragraph nil)
>   c-mask-comment(t nil fill-paragraph nil)
>   c-fill-paragraph(nil)
>   call-interactively(c-fill-paragraph)

I believe I've fixed this bug now, thanks for reporting it.
I still think it's also a bug for cc-mode to rebind M-q instead of
setting fill-paragraph-function.  Or is there a good reason for
doing it this way ?


	Stefan



-------------------------------------------------------
This SF.NET email is sponsored by: Thawte.com - A 128-bit supercerts will
allow you to extend the highest allowed 128 bit encryption to all your 
clients even if they use browsers that are limited to 40 bit encryption. 
Get a guide here:http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0030en

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

[Robert Anderson]

--- Original Message ---
From: "Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu>
To: "Robert Anderson" <RWA@sbcglobal.net>, bug-cc-mode@gnu.org
CC: emacs-devel@gnu.org
Subject: Re: regression: filling comments in C++ code (today's CVS) 

>> Debugger entered--Lisp error: (search-failed "/\\*+ *\\|//+ *")
>>   re-search-forward("/\\*+ *\\|//+ *")
>>   fill-comment-paragraph(nil)
>>   fill-paragraph(nil)
>>   apply(fill-paragraph nil)
>>   c-mask-comment(t nil fill-paragraph nil)
>>   c-fill-paragraph(nil)
>>   call-interactively(c-fill-paragraph)
>
>I believe I've fixed this bug now, thanks for reporting it.

Is the fix in CVS?  I can run my tests on it if so.  Thanks for
responding to my report.

>I still think it's also a bug for cc-mode to rebind M-q instead of
>setting fill-paragraph-function.  Or is there a good reason for
>doing it this way ?

I could say "I doubt it," but I honestly have no idea.

Thanks,
Bob

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

[Robert Anderson]

--- Original Message ---
From: "Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu>
To: "Robert Anderson" <RWA@sbcglobal.net>, bug-cc-mode@gnu.org
CC: emacs-devel@gnu.org
Subject: Re: regression: filling comments in C++ code (today's CVS) 

>> Debugger entered--Lisp error: (search-failed "/\\*+ *\\|//+ *")
>>   re-search-forward("/\\*+ *\\|//+ *")
>>   fill-comment-paragraph(nil)
>>   fill-paragraph(nil)
>>   apply(fill-paragraph nil)
>>   c-mask-comment(t nil fill-paragraph nil)
>>   c-fill-paragraph(nil)
>>   call-interactively(c-fill-paragraph)
>
>I believe I've fixed this bug now, thanks for reporting it.

(~/emacs-test)$ ./run-tests 
GNU Emacs 21.3.50.7
./c++-mode/c-fill-paragraph/indented-comments: filling indented
comments... FAIL.
./c++-mode/c-fill-paragraph/left-flush-comments: filling left
flush comments... ok.
./fortran-mode/fill-paragraph/comments: filling comments... ok.
Failed: 1
(~/emacs-test)$ ./run-tests 
GNU Emacs 21.3.50.8
./c++-mode/c-fill-paragraph/indented-comments: filling indented
comments... ok.
./c++-mode/c-fill-paragraph/left-flush-comments: filling left
flush comments... ok.
./fortran-mode/fill-paragraph/comments: filling comments... ok.
All tests passed.

Indeed, you have fixed the bug I reported.  Thanks!

Bob





-------------------------------------------------------
This SF.NET email is sponsored by: Thawte.com - A 128-bit supercerts will
allow you to extend the highest allowed 128 bit encryption to all your 
clients even if they use browsers that are limited to 40 bit encryption. 
Get a guide here:http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0030en

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

[Richard Stallman]

    Anyway, it's obviously a problem in my new filling code.
    I'll take a look at it, but in the mean time, rebinding
    M-q to fill-paragraph (in c-common-mode-hook I guess)
    and setting fill-paragraph-function to `c-fill-paragraph'
    should hide the problem.

If this is the right thing to do, and if the CC mode maintainer
doesn't respond soon, could you change it?


-------------------------------------------------------
This SF.NET email is sponsored by: Thawte.com - A 128-bit supercerts will
allow you to extend the highest allowed 128 bit encryption to all your 
clients even if they use browsers that are limited to 40 bit encryption. 
Get a guide here:http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0030en

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

[Martin Stjernholm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> wrote:

> Why on earth does cc-mode rebind M-q instead of setting
> fill-paragraph-function ?

Because c-fill-paragraph doesn't do the actual filling. It masks the
comment or string the point is within and calls the normal fill
function to make the appropriate adaptive filling etc. For this to
work it must run first, before any other code has tried to do things
like calculate the fill prefix and thereby clobbered its true value.

Specifically, the filladapt package replaces fill-paragraph with its
own variant that calculates the adaptive fill prefix before calling
fill-paragraph-function. CC Mode must do the comment masking before
that to make it adapt to the proper prefix.



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

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

[Stefan Monnier]

> > Why on earth does cc-mode rebind M-q instead of setting
> > fill-paragraph-function ?
> 
> Because c-fill-paragraph doesn't do the actual filling. It masks the
> comment or string the point is within and calls the normal fill
> function to make the appropriate adaptive filling etc. For this to
> work it must run first, before any other code has tried to do things
> like calculate the fill prefix and thereby clobbered its true value.

But that's what fill-paragraph does.
It first calls fill-paragraph-function.

> Specifically, the filladapt package replaces fill-paragraph with its
> own variant that calculates the adaptive fill prefix before calling
> fill-paragraph-function. CC Mode must do the comment masking before
> that to make it adapt to the proper prefix.

Ah, so it's because filladapt does not faithfully implement the
fill-paragraph interface.

For the sake of correctness in cases other than filladapt, how about
setting fill-paragraph-function (while still keeping the rebinding
of M-q) so that calling fill-paragraph DTRT.


	Stefan

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

[Martin Stjernholm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> wrote:

> Ah, so it's because filladapt does not faithfully implement the
> fill-paragraph interface.

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.

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
keep a fill postfix or to fill inside a rectangle. Doing the actual
filling is pretty messy, so other packages rather device various
kludges to avoid duplicating that code.

> For the sake of correctness in cases other than filladapt, how about
> setting fill-paragraph-function (while still keeping the rebinding
> of M-q) so that calling fill-paragraph DTRT.

That wouldn't work when filladapt is used, but it's perhaps good
enough for your purposes. A more thorough solution would be to put an
advice around fill-paragraph.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

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

[Richard Stallman]

    That wouldn't work when filladapt is used, but it's perhaps good
    enough for your purposes. A more thorough solution would be to put an
    advice around fill-paragraph.

We want to avoid having Emacs packages advise Emacs functions, because
that makes the code hard to understand.  An explicit hook is clearer,
because you can see the code that calls the hook and then you know
something strange might happen there.

    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
    keep a fill postfix or to fill inside a rectangle. Doing the actual
    filling is pretty messy, so other packages rather device various
    kludges to avoid duplicating that code.

If this is useful, I'm in favor it in principle.  It could make the
code more modular.


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

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

[Stefan Monnier]

> > Ah, so it's because filladapt does not faithfully implement the
> > fill-paragraph interface.
> 
> 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 ?

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

> keep a fill postfix or to fill inside a rectangle.

This part is indeed missing, obviously.

> > For the sake of correctness in cases other than filladapt, how about
> > setting fill-paragraph-function (while still keeping the rebinding
> > of M-q) so that calling fill-paragraph DTRT.
> 
> That wouldn't work when filladapt is used, but it's perhaps good

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.


	Stefan



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

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

[Martin Stjernholm]

[Sorry for the late reply.]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> wrote:

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

That'd be workable, but it's not a very pretty solution. 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).

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

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

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

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.

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.

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

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



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

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

[Martin Stjernholm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> 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.

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

[Stefan Monnier]

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

Since the function should basically never be called on any empty line
at all, I have no idea why blank lines are skipped like that and
neither do I know when it would harm.

Could you describe in which context you bumped into it ?

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

I consider selective-display as an obsolete feature, so I don't think
we should try to improve support for it.


	Stefan

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

[Martin Stjernholm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> wrote:

> > 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.
> 
> Since the function should basically never be called on any empty line
> at all /.../

That provision isn't obvious to me. The doc says "it removes any
paragraph breaks in the region/.../". I interpret "paragraph breaks"
to include empty lines in the region, at the start as much as anywhere
else.

> Could you describe in which context you bumped into it ?

I only bumped into it because I took a quick look at the function, and
it looked like something that could surprise programmers that embed
fill-region-as-paragraph and thus cause their code to misbehave in
some odd situation down the road. According to the principle of least
surprising behavior one should ask for specific reasons to have a
special case rather than reasons for not having it.

The only reason I can see to have this particular one is that it's
already there and has been for some time. Perhaps that's a good enough
cause to keep it.



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

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

[Stefan Monnier]

> > > 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.
> > 
> > Since the function should basically never be called on any empty line
> > at all /.../
> 
> That provision isn't obvious to me.

The above "should" was not meant as a recommendation but as a comment
on the typical way the function is used.

> The doc says "it removes any
> paragraph breaks in the region/.../". I interpret "paragraph breaks"
> to include empty lines in the region, at the start as much as anywhere
> else.

Agreed.

> > Could you describe in which context you bumped into it ?
> 
> I only bumped into it because I took a quick look at the function, and
> it looked like something that could surprise programmers that embed
> fill-region-as-paragraph and thus cause their code to misbehave in
> some odd situation down the road. According to the principle of least
> surprising behavior one should ask for specific reasons to have a
> special case rather than reasons for not having it.
> 
> The only reason I can see to have this particular one is that it's
> already there and has been for some time. Perhaps that's a good enough
> cause to keep it.

Just like you I don't understand why those lines are there.
Maybe it's related to the fact that backward-paragraph will skip
the the empty line before the paragraph (if there is such an empty
line), but in that case, the blank-line-skip should probably
happen in fill-paragraph rather than in fill-region-as-paragraph.


	Stefan