[Qemu-devel] Patch removing spaces

[Qemu-devel] Patch removing spaces

[Balazs Attila-Mihaly (Cd-MaN)]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

Hello all,

If anyone is interested: a patch which removes spaces from the end of the lines. Compressed because it's rather big.

Best regards.



      __________________________________________________________
Sent from Yahoo! Mail.
A Smarter Email http://uk.docs.yahoo.com/nowyoucan.html

[-- Attachment #2: remove_spaces.diff.bz2 --]
[-- Type: application/x-bzip, Size: 33863 bytes --]

Re: [Qemu-devel] Patch removing spaces

[Ian Jackson]

Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
> spaces"): If anyone is interested: a patch which removes spaces from
> the end of the lines. Compressed because it's rather big.

Please don't apply this patch!

Spaces at the ends of lines are indeed irritating and we should avoid
them.  But going out of our way to remove them is definitely wrong -
it's reformatting, which is nearly always wrong in Free Software.

See for example the CVS usage guidelines that Tony Finch recommends
for use in the FreeBSD project
  http://dotat.at/writing/cvs-guidelines.html
section 3.1.

Ian.

Re: [Qemu-devel] Patch removing spaces

[Johannes Schindelin]

Hi,

On Mon, 19 May 2008, Ian Jackson wrote:

> Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
> > spaces"): If anyone is interested: a patch which removes spaces from 
> > the end of the lines. Compressed because it's rather big.
> 
> Please don't apply this patch!

In any case, _if_ something like this should be committed, I'd _rather_ 
have a small script that is easily verifyable, instead of a big patch that 
can hide too many things.

Don't get me wrong, I do not distrust "Cd-MaN", but, well, better be safe 
than sorry, right?

Ciao,
Dscho

[Qemu-devel] Re: Patch removing spaces

[Jan Kiszka]

Ian Jackson wrote:
> Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
>> spaces"): If anyone is interested: a patch which removes spaces from
>> the end of the lines. Compressed because it's rather big.
> 
> Please don't apply this patch!
> 
> Spaces at the ends of lines are indeed irritating and we should avoid
> them.  But going out of our way to remove them is definitely wrong -
> it's reformatting, which is nearly always wrong in Free Software.

Reformatting code according to some well established coding style is
surely _not_ wrong - for whatever kind of software. Tools are smart
enough to ignore plain whitespace changes. But even if rewrapping is
required, a clear commit comment will keep people away from searching
for functional changes (and that there are non can be checked beforehand
at binary level).

Reformatting sometimes also means re-reviewing (though I guess not in
this case) with the chance to discover yet hidden bugs. Happened quite a
few times for Linux during recent coding style fixes. Of course, bug
fixes were not merged with style fixes.

But such changes are indeed pointless if there is no decision to
strictly enforce the coding style from that point on. And if there is no
clearly documented style guide (e.g. in form of a indent script + a few
relaxation rules).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Patch removing spaces

[malc]

On Mon, 19 May 2008, Ian Jackson wrote:

> Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
>> spaces"): If anyone is interested: a patch which removes spaces from
>> the end of the lines. Compressed because it's rather big.
>
> Please don't apply this patch!
>
> Spaces at the ends of lines are indeed irritating and we should avoid
> them.  But going out of our way to remove them is definitely wrong -
> it's reformatting, which is nearly always wrong in Free Software.
>

[..snip..]

Preventive measure that makes the code look sufficiently ugly when
tabs or trailing whitespace is present. Hands almost reflectively
reach for `M-x nuke-trailing-whitespace' and `C-x h M-x untabify'

(defface font-lock-ws-face
   '((((class color) (background dark)) (:background "white"))
     (((class color) (background light)) (:background "black"))
     (t (:bold t)))
   "Font Lock mode face used to highlight unwated whitespace."
   :group 'font-lock-faces)
(setq font-lock-ws-face 'font-lock-ws-face)

(defun font-lock-mode-hook-fn ()
   (setq old-font-lock-keywords font-lock-keywords)
   (make-variable-buffer-local 'font-lock-keywords)
   (let ((elem
          (list
           (list "\\([ \t]+$\\)\\|\\(\t\\)"
                 (list 0 font-lock-ws-face)))))
     (setq font-lock-keywords (append font-lock-keywords elem))))

(add-hook 'font-lock-mode-hook 'font-lock-mode-hook-fn)

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Patch removing spaces

[Blue Swirl]

On 5/19/08, malc <av1474@comtv.ru> wrote:
> On Mon, 19 May 2008, Ian Jackson wrote:
>
>
> > Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
> >
> > > spaces"): If anyone is interested: a patch which removes spaces from
> > > the end of the lines. Compressed because it's rather big.
> > >
> >
> > Please don't apply this patch!
> >
> > Spaces at the ends of lines are indeed irritating and we should avoid
> > them.  But going out of our way to remove them is definitely wrong -
> > it's reformatting, which is nearly always wrong in Free Software.
> >
> >
>
>  [..snip..]
>
>  Preventive measure that makes the code look sufficiently ugly when
>  tabs or trailing whitespace is present. Hands almost reflectively
>  reach for `M-x nuke-trailing-whitespace' and `C-x h M-x untabify'

No match for M-x nuke-trailing-whitespace, where is that?

>  (defface font-lock-ws-face
>   '((((class color) (background dark)) (:background "white"))
>     (((class color) (background light)) (:background "black"))
>     (t (:bold t)))
>   "Font Lock mode face used to highlight unwated whitespace."
>   :group 'font-lock-faces)
>  (setq font-lock-ws-face 'font-lock-ws-face)
>
>  (defun font-lock-mode-hook-fn ()
>   (setq old-font-lock-keywords font-lock-keywords)
>   (make-variable-buffer-local 'font-lock-keywords)
>   (let ((elem
>          (list
>           (list "\\([ \t]+$\\)\\|\\(\t\\)"
>                 (list 0 font-lock-ws-face)))))
>     (setq font-lock-keywords (append font-lock-keywords elem))))
>
>  (add-hook 'font-lock-mode-hook 'font-lock-mode-hook-fn)

Thanks, it's ugly!

Re: [Qemu-devel] Patch removing spaces

[Stuart Brady]

On Mon, May 19, 2008 at 09:14:00PM +0300, Blue Swirl wrote:
> Thanks, it's ugly!

Ugly, but neat! :)

BTW, for those using VIM, place this in your .vimrc:

   highlight WhiteSpaceEOL ctermbg=darkgreen guibg=lightgreen
   match WhiteSpaceEOL /\s\+$/
   autocmd WinEnter * match WhiteSpaceEOL /\s\+$/

Taken from http://www.vim.org/tips/tip.php?tip_id=1040, and modified
not to highlight leading spaces -- perhaps highlighting groups of eight
or more leading spaces would be useful, though?

HTH,
-- 
Stuart Brady

Re: [Qemu-devel] Patch removing spaces

[Paul Brook]

> Taken from http://www.vim.org/tips/tip.php?tip_id=1040, and modified
> not to highlight leading spaces -- perhaps highlighting groups of eight
> or more leading spaces would be useful, though?

No. Qemu uses spaces, not tabs, so you'd want to highlight tab characters if 
anything.

Paul

Re: [Qemu-devel] Patch removing spaces

[Laurent Desnogues]

On 5/19/08, Paul Brook <paul@codesourcery.com> wrote:
> No. Qemu uses spaces, not tabs, so you'd want to highlight tab characters if
>  anything.

I use this for emacs:

(setq-default indent-tabs-mode nil)
(setq-default show-trailing-whitespace t)


Laurent

Re: [Qemu-devel] Patch removing spaces

[malc]

On Mon, 19 May 2008, Blue Swirl wrote:

> On 5/19/08, malc <av1474@comtv.ru> wrote:
>> On Mon, 19 May 2008, Ian Jackson wrote:
>>
>>
>>> Balazs Attila-Mihaly \(Cd-MaN\) writes ("[Qemu-devel] Patch removing
>>>
>>>> spaces"): If anyone is interested: a patch which removes spaces from
>>>> the end of the lines. Compressed because it's rather big.
>>>>
>>>
>>> Please don't apply this patch!
>>>
>>> Spaces at the ends of lines are indeed irritating and we should avoid
>>> them.  But going out of our way to remove them is definitely wrong -
>>> it's reformatting, which is nearly always wrong in Free Software.
>>>
>>>
>>
>>  [..snip..]
>>
>>  Preventive measure that makes the code look sufficiently ugly when
>>  tabs or trailing whitespace is present. Hands almost reflectively
>>  reach for `M-x nuke-trailing-whitespace' and `C-x h M-x untabify'
>
> No match for M-x nuke-trailing-whitespace, where is that?

Oh, i have had it in my elisp collection for so long that forgot that
it's not included in XEmacs (not sure about FSF Emacs)

That's the one i use:

;; whitespace.el --- strip trailing whitespace from buffers
;; Author: Noah Friedman <friedman@prep.ai.mit.edu>
;; Created: 1995-02-11

>>  (defface font-lock-ws-face
>>   '((((class color) (background dark)) (:background "white"))
>>     (((class color) (background light)) (:background "black"))
>>     (t (:bold t)))
>>   "Font Lock mode face used to highlight unwated whitespace."
>>   :group 'font-lock-faces)
>>  (setq font-lock-ws-face 'font-lock-ws-face)
>>
>>  (defun font-lock-mode-hook-fn ()
>>   (setq old-font-lock-keywords font-lock-keywords)
>>   (make-variable-buffer-local 'font-lock-keywords)
>>   (let ((elem
>>          (list
>>           (list "\\([ \t]+$\\)\\|\\(\t\\)"
>>                 (list 0 font-lock-ws-face)))))
>>     (setq font-lock-keywords (append font-lock-keywords elem))))
>>
>>  (add-hook 'font-lock-mode-hook 'font-lock-mode-hook-fn)
>
> Thanks, it's ugly!

You are welcome :)

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Patch removing spaces

[Balazs Attila-Mihaly (Cd-MaN)]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hello all

I had no idea that the patch would start such a long discussion :-). I fully agree with the comment that a script producing the output is preferred to the actual patch, so that it can be verified. Attached you will find the perl script (which is a slight variation of the script I posted the link to earlier) used to perform the operation. It should be launched from the source directory and will go recursively (excluding .svn directories) and search for .c, .h and .texi files and apply the space-removing process on them. In the same time it replaces Windows newlines with *nix newlines (there were a couple of those in the source).

Don't get me wrong, I'm not pushing for inclusion or anything like that, I just thought that this was a convention in the source and wanted to help out preserving it.

Best regards.



      __________________________________________________________
Sent from Yahoo! Mail.
A Smarter Email http://uk.docs.yahoo.com/nowyoucan.html

[-- Attachment #2: strip_spaces.pl --]
[-- Type: application/x-perl, Size: 917 bytes --]

Re: [Qemu-devel] Patch removing spaces

[Ian Jackson]

Balazs Attila-Mihaly \(Cd-MaN\) writes ("Re: [Qemu-devel] Patch removing spaces"):
> Don't get me wrong, I'm not pushing for inclusion or anything like
> that, I just thought that this was a convention in the source and
> wanted to help out preserving it.

_Preserving_ the convention is fine.  I agree that patches which
introduce trailing whitespace (or carriage returns!) should not be
commited.

But going through and changing the existing code is a very bad idea.
The benefits are trivial and the costs (extra conflicts, spurious
output from `vcs blame', spurious diffs reported, and so on) are
substantial.

Changing the existing code would be a bad idea even if it was to
improve a layout style which everyone agreed was horrid and confusing
to work with.  It is infinitely more of a bad idea when the problem,
and thus the fix, is actually invisible and so practically worthless.

Ian.