[PATCHv2] Vimrc config with GNU formatting

Hi all,

This is a second version of patch which adds a Vim config (.local.vimrc)
to root folder to allow automatic setup of GNU formatting for 
C/C++/Java/Lex GCC files.

I've updated the code with comments from Richard and Bernhard (which 
fixed formatting
of lonely closing bracket).

The patch caused a lively debate with Segher who wanted .local.vimrc to 
not be enabled
by default. We basically have two options:
1) put .local.vimrc to root (just like .dir-locals.el config for Emacs)
2) put both .local.vimrc and .dir-locals.el to contrib and add Makefile 
targets
to create symlinks in root folder per user's request
I personally prefer 2) because this would IMHO improve the quality of 
patches
(e.g. no more silly tab-whitespace formatting bugs).

Thoughts? Ok to commit?

-Y

> I personally prefer 2)

Sorry, I meant 1) of course that is enable vimrc config by default.

-Y

On Wed, Sep 10, 2014 at 12:09:25PM +0400, Yury Gribov wrote:
> The patch caused a lively debate with Segher who wanted .local.vimrc to 
> not be enabled
> by default.

No, that is not what I said.  I am saying it is very anti-social to make
people's editor behave differently from what they are used to.  Wars have
been started for much less.

But there are more problems with your approach.  First, you are encouraging
the use of a plugin that is a gaping wide security hole.  I do not think
GCC should encourage this.

Secondly, this is a very poor imitation of the mechanism Vim has for dealing
with filetypes, namely, ftplugins.

> We basically have two options:
> 1) put .local.vimrc to root (just like .dir-locals.el config for Emacs)
> 2) put both .local.vimrc and .dir-locals.el to contrib and add Makefile 
> targets
> to create symlinks in root folder per user's request

As I said before, Emacs is not Vim.  The Emacs dir-locals file simply
configures some settings for files with certain major modes in that dir.
For example, ours says that c-mode files should use GNU style.  This is
quite harmless, and probably what most Emacs users want.

Your script on the other hand does something totally different: it overrides
a bunch of settings the user has made elsewhere.

[Snipped some overly optimistic stuff about this all increasing the quality
of posted patches.  Hint: the most frequently made formatting error is
forgetting to put two spaces at the end of a sentence.  Which this script
does not handle at all (I'm not saying it should, it is hard to do reliably).
The patch itself however does introduce another one of these.]


Segher

Segher Boessenkool <segher <at> kernel.crashing.org> writes:
> I am saying it is very anti-social to make
> people's editor behave differently from what they are used to.
> ...
> The Emacs dir-locals file simply
> configures some settings for files with certain major modes in that 
dir.
> For example, ours says that c-mode files should use GNU style.  This 
is
> quite harmless, and probably what most Emacs users want.

Hm, so autoformatting in Emacs is good because that's what most users 
want but autoformatting in Vim is bad because that's not what people are 
used to?

> First, you are encouraging
> the use of a plugin that is a gaping wide security hole.

I don't think so. The comment mentions that user can either install a 
(rather widespread btw) plugin or just call config from his .vimrc.

> Secondly, this is a very poor imitation of the mechanism Vim has for 
dealing
> with filetypes, namely, ftplugins.

I'm ready to accept technical suggestions on how to do the thing 
properly. So what exactly do you propose?

> [Snipped some overly optimistic stuff about this all increasing the 
quality
> of posted patches.  Hint: the most frequently made formatting error is
> forgetting to put two spaces at the end of a sentence.

Dunno, I was relying on personal experience. And searching for "two|2 
spaces" on http://gcc.gnu.org/ml/gcc-patches returns 2000 results 
whereas "eight|8 spaces" only 700.

-Y

On Wed, Sep 10, 2014 at 10:09 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> Hi all,
>
> This is a second version of patch which adds a Vim config (.local.vimrc)
> to root folder to allow automatic setup of GNU formatting for C/C++/Java/Lex
> GCC files.
>
> I've updated the code with comments from Richard and Bernhard (which fixed
> formatting
> of lonely closing bracket).
>
> The patch caused a lively debate with Segher who wanted .local.vimrc to not
> be enabled
> by default. We basically have two options:
> 1) put .local.vimrc to root (just like .dir-locals.el config for Emacs)
> 2) put both .local.vimrc and .dir-locals.el to contrib and add Makefile
> targets
> to create symlinks in root folder per user's request
> I personally prefer 2) because this would IMHO improve the quality of
> patches
> (e.g. no more silly tab-whitespace formatting bugs).
>
> Thoughts? Ok to commit?

It doesn't handle indenting switch/case correctly.  I get

 switch (x)
   {
   case X:
          {
             int foo;
...

that is, the { after the case label is wrongly indented.  The same happens
for
  {
       {
       }
  }

we seem to get two soft-tabs here.

Richard.

> -Y
>
>
>

> On Sep 10, 2014, at 9:47 PM, Yury Gribov <tetra2005@gmail.com> wrote:
> 
> Segher Boessenkool <segher <at> kernel.crashing.org> writes:
>> I am saying it is very anti-social to make
>> people's editor behave differently from what they are used to.
>> ...
>> The Emacs dir-locals file simply
>> configures some settings for files with certain major modes in that
> dir.
>> For example, ours says that c-mode files should use GNU style.  This
> is
>> quite harmless, and probably what most Emacs users want.
> 
> Hm, so autoformatting in Emacs is good because that's what most users 
> want but autoformatting in Vim is bad because that's not what people are 
> used to?

I don't like auto formatting in any editor. Though I don't use emacs, I use vim. I think using auto formatting is cheating and not understanding why coding styles exists.  And some folks already have to deal with two more formatting styles already: Linux kernel and gnu. So if you add auto formatting to one, some folks are going to get confused. 


Thanks,
Andrew

> 
>> First, you are encouraging
>> the use of a plugin that is a gaping wide security hole.
> 
> I don't think so. The comment mentions that user can either install a 
> (rather widespread btw) plugin or just call config from his .vimrc.
> 
>> Secondly, this is a very poor imitation of the mechanism Vim has for
> dealing
>> with filetypes, namely, ftplugins.
> 
> I'm ready to accept technical suggestions on how to do the thing 
> properly. So what exactly do you propose?
> 
>> [Snipped some overly optimistic stuff about this all increasing the
> quality
>> of posted patches.  Hint: the most frequently made formatting error is
>> forgetting to put two spaces at the end of a sentence.
> 
> Dunno, I was relying on personal experience. And searching for "two|2 
> spaces" on http://gcc.gnu.org/ml/gcc-patches returns 2000 results 
> whereas "eight|8 spaces" only 700.
> 
> -Y
>

On Thu, Sep 11, 2014 at 11:06 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Sep 10, 2014 at 10:09 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>> Hi all,
>>
>> This is a second version of patch which adds a Vim config (.local.vimrc)
>> to root folder to allow automatic setup of GNU formatting for C/C++/Java/Lex
>> GCC files.
>>
>> I've updated the code with comments from Richard and Bernhard (which fixed
>> formatting
>> of lonely closing bracket).
>>
>> The patch caused a lively debate with Segher who wanted .local.vimrc to not
>> be enabled
>> by default. We basically have two options:
>> 1) put .local.vimrc to root (just like .dir-locals.el config for Emacs)
>> 2) put both .local.vimrc and .dir-locals.el to contrib and add Makefile
>> targets
>> to create symlinks in root folder per user's request
>> I personally prefer 2) because this would IMHO improve the quality of
>> patches
>> (e.g. no more silly tab-whitespace formatting bugs).
>>
>> Thoughts? Ok to commit?
>
> It doesn't handle indenting switch/case correctly.  I get
>
>  switch (x)
>    {
>    case X:
>           {
>              int foo;
> ...
>
> that is, the { after the case label is wrongly indented.  The same happens
> for
>   {
>        {
>        }
>   }
>
> we seem to get two soft-tabs here.


    setlocal cinoptions=>s,n-s,{s,:s,=s,g0,hs,p5,t0,+s,(0,u0,w1,m0

does better but still oddly handles

  switch (x)
    {
    case X:
        {
        tree x;

thus indents a brace two spaces too much (but the stmts are
correctly indented).  The following is handled fine:

  switch (x)
    {
    case X:
       foo ();

Richard.

> Richard.
>
>> -Y
>>
>>
>>

On 09/11/2014 01:14 PM, pinskia@gmail.com wrote:
> I don't like auto formatting in any editor.

Ok, so +1 for moving to contrib/.

> And some folks already have to deal with two more formatting styles already:
> Linux kernel and gnu. So if you add auto formatting to one,
>some folks are going to get confused.

This config will only turn on autoformatting for GCC sources.
It's not going to influence other directories.

-Y

On 09/11/2014 01:18 PM, Richard Biener wrote:
On Thu, Sep 11, 2014 at 11:06 AM, Richard Biener
<richard.guenther@gmail.com>  wrote:
> >On Wed, Sep 10, 2014 at 10:09 AM, Yury Gribov<y.gribov@samsung.com>  wrote:
>> >>Hi all,
>> >>
>> >>This is a second version of patch which adds a Vim config (.local.vimrc)
>> >>to root folder to allow automatic setup of GNU formatting for C/C++/Java/Lex
>> >>GCC files.
>> >>
>> >>I've updated the code with comments from Richard and Bernhard (which fixed
>> >>formatting
>> >>of lonely closing bracket).
>> >>
>> >>The patch caused a lively debate with Segher who wanted .local.vimrc to not
>> >>be enabled
>> >>by default. We basically have two options:
>> >>1) put .local.vimrc to root (just like .dir-locals.el config for Emacs)
>> >>2) put both .local.vimrc and .dir-locals.el to contrib and add Makefile
>> >>targets
>> >>to create symlinks in root folder per user's request
>> >>I personally prefer 2) because this would IMHO improve the quality of
>> >>patches
>> >>(e.g. no more silly tab-whitespace formatting bugs).
>> >>
>> >>Thoughts? Ok to commit?
> >
> >It doesn't handle indenting switch/case correctly.  I get
> >
> >  switch (x)
> >    {
> >    case X:
> >           {
> >              int foo;
> >...
> >
> >that is, the { after the case label is wrongly indented.  The same happens
> >for
> >   {
> >        {
> >        }
> >   }
> >
> >we seem to get two soft-tabs here.
>
>     setlocal cinoptions=>s,n-s,{s,:s,=s,g0,hs,p5,t0,+s,(0,u0,w1,m0
>
> does better but still oddly handles

Also fails for

   if (1)
     {
     x = 2;
     }

On 09/11/2014 02:10 PM, Yury Gribov wrote:
> On 09/11/2014 01:18 PM, Richard Biener wrote:
> On Thu, Sep 11, 2014 at 11:06 AM, Richard Biener
> <richard.guenther@gmail.com>  wrote:
>> >On Wed, Sep 10, 2014 at 10:09 AM, Yury Gribov<y.gribov@samsung.com>
>> wrote:
>>> >>Hi all,
>>> >>
>>> >>This is a second version of patch which adds a Vim config
>>> (.local.vimrc)
>>> >>to root folder to allow automatic setup of GNU formatting for
>>> C/C++/Java/Lex
>>> >>GCC files.
>>> >>
>>> >>I've updated the code with comments from Richard and Bernhard
>>> (which fixed
>>> >>formatting
>>> >>of lonely closing bracket).
>>> >>
>>> >>The patch caused a lively debate with Segher who wanted
>>> .local.vimrc to not
>>> >>be enabled
>>> >>by default. We basically have two options:
>>> >>1) put .local.vimrc to root (just like .dir-locals.el config for
>>> Emacs)
>>> >>2) put both .local.vimrc and .dir-locals.el to contrib and add
>>> Makefile
>>> >>targets
>>> >>to create symlinks in root folder per user's request
>>> >>I personally prefer 2) because this would IMHO improve the quality of
>>> >>patches
>>> >>(e.g. no more silly tab-whitespace formatting bugs).
>>> >>
>>> >>Thoughts? Ok to commit?
>> >
>> >It doesn't handle indenting switch/case correctly.  I get
>> >
>> >  switch (x)
>> >    {
>> >    case X:
>> >           {
>> >              int foo;
>> >...
>> >
>> >that is, the { after the case label is wrongly indented.  The same
>> happens
>> >for
>> >   {
>> >        {
>> >        }
>> >   }
>> >
>> >we seem to get two soft-tabs here.
>>
>>     setlocal cinoptions=>s,n-s,{s,:s,=s,g0,hs,p5,t0,+s,(0,u0,w1,m0
>>
>> does better but still oddly handles
>
> Also fails for
>
>    if (1)
>      {
>      x = 2;
>      }

Ok, it tooks some time. Basically we want brace symbol to behave 
differently in two contexts:

1) not add any additional offset when not following control flow operator:
void
f ()
{
   int x;
   {
   }
}

2) but add shifttab otherwise:
void
f()
{
   if (1)
     {
     }
}

My understanding is that {N looks too rigid and always adds the same 
amount to current indent. Thus we see parasitic whites in the first case.

I wonder what would be the best way to handle this. We could just live 
with that (free {}'s are rare anyway) or I could try to hack a custom 
indentexpr (this will of course increase the complexity of patch).

-Y

On Thu, 11 Sep 2014, Yury Gribov wrote:
> Ok, it tooks some time. Basically we want brace symbol to behave differently
> in two contexts:
> 
> 1) not add any additional offset when not following control flow operator:
> void
> f ()
> {
>   int x;
>   {
>   }
> }

Note that GCC commonly uses custom iteration macros, e.g.:

  FOR_EACH_BB_FN(bb, fn)
    {
      do_stuff;
    }

and cinoptions that get the braces-in-switch case wrong should get constructs
like the above right.

(to get gnu-style autoindent in Vim, I've been using
http://www.vim.org/scripts/script.php?script_id=575 and would adjust braces in
switch by hand if need arose; the script is probably very close to one of
approaches posted in this thread, but I haven't checked)

Alexander

On 09/14/2014 02:30 PM, Alexander Monakov wrote:
> On Thu, 11 Sep 2014, Yury Gribov wrote:
>> Ok, it tooks some time. Basically we want brace symbol to behave differently
>> in two contexts:
>>
>> 1) not add any additional offset when not following control flow operator:
>> void
>> f ()
>> {
>>    int x;
>>    {
>>    }
>> }
>
> Note that GCC commonly uses custom iteration macros, e.g.:
>
>    FOR_EACH_BB_FN(bb, fn)
>      {
>        do_stuff;
>      }
>
> and cinoptions that get the braces-in-switch case wrong should get constructs
> like the above right.

Right, looks like a fair tradeoff.

> (to get gnu-style autoindent in Vim, I've been using
> http://www.vim.org/scripts/script.php?script_id=575 and would adjust braces in
> switch by hand if need arose; the script is probably very close to one of
> approaches posted in this thread, but I haven't checked)

Thanks for sharing, these Vim configs just keep popping up. Here are 
results of quick analysis.

Plugin's pros:
* KR-style parameters are 4 chars instead of 5 (not sure that's 
important for GCC but matters in e.g. zlib so makes sense to borrow; 
I'll update next version of patch with this)
* explicitly set f0 in cino (again makes sense to borrow)

And cons:
* sets formatting globally for all *.c files
* only works for C sources and headers so e.g. won't work for libstdc++
* does not know about C++ formatting (e.g. public:, etc.)
* continuation line is indented by 1 char instead of GCC's 2

And also some stuff which we probably don't care about:
* plugin does not use softtabstop so Tab inserts a real tab character
* better formatting of special type of comments:
/* Start
  * Middle
  */
* sets up browsefilter on MS platforms

-Y