On Wed, Sep 17, 2014 at 09:08:44PM +0400, Yury Gribov wrote:
[ Use a proper mime type and target-disposition inline, as contribute.html
tells you. Or use a saner email client; since you're using git, try git
send-email perhaps? Thanks. ]
> index f7c7e38..ee2321f 100644
> --- a/Makefile.tpl
> +++ b/Makefile.tpl
> @@ -867,6 +867,11 @@ mail-report-with-warnings.log: warning.log
> chmod +x $@
> echo If you really want to send e-mail, run ./$@ now
>
> +# Local Vim config
> +
> +vimrc:
> + (cd $(srcdir); $(LN_S) contrib/vimrc .local.vimrc; $(LN_S)
> contrib/vimrc .lvimrc)
> +
This is another target than what the doc (in the script itself) mentions.
It is not marked as phony. It should not _be_ phony; the two files should
be separate targets. Why make links instead of copies? A user will likely
want to edit his config. The way you use ";" is wrong (it continues if there
is an error). You don't need the "cd" anyway, come to that.
It's pretty silly to have a makefile target that only copies a file (that
is never used by the makefile itself); just tell in the doc where to copy
the file.
> --- /dev/null
> +++ b/contrib/vimrc
> @@ -0,0 +1,45 @@
> +" Code formatting settings for Vim.
> +"
> +" To enable this for GCC files by default, install thinca's vim-localrc
> +" plugin and do
> +" $ make .local.vimrc
No, we should *not* advertise an "enough rope" solution without mentioning
it *will* kill you. Or not mention it at all. Esp. since your next option
has all the same functionality and more.
> +" Or install Markus Braun's localvimrc and do
> +" $ make .lvimrc
As said before, these targets won't work.
> +" Or if you dislike plugins, add autocmd in your ~/.vimrc:
> +" :au BufNewFile,BufReadPost path/to/gcc/* :so path/to/gcc/contrib/vimrc
There are many more reasons than just "dislike of plugins" to prefer
something like this. For one thing, many Vim users will have many
similar statements in their config _already_.
> +" Or just source file manually every time if you are masochist:
> +" :so path/to/gcc/contrib/vimrc
How is that masochist? Typing that cino by hand though, now that would
qualify ;-)
Your list reads as a recommendation, telling the reader to only do the
latter options if they are desperate/stupid/etc. While it really is
the other way around: only lazy people or people who cannot configure
their editor themselves want the do-stuff-behind-your-back solution.
Just keep things neutral please.
> + setlocal cindent
> + setlocal shiftwidth=2
> + setlocal softtabstop=2
> + setlocal cinoptions=>2s,n-s,{s,^-s,:s,=s,g0,f0,hs,p2s,t0,+s,(0,u0,w1,m0
If you write this as absolute numbers instead of as shift widths, you do not
need to force sw and sts settings down people's throat. It might also be
easier to read? Well I doubt that, but it will be slightly shorter at least.
> + setlocal textwidth=79
The coding conventions say maximum line length is 80. 'tw' is a user
preference as well, and this one is almost as annoying as cindent, if
not more.
> + setlocal formatoptions-=ro formatoptions+=cql
Yet another user preference. Also mostly the default, except "l" -- which
won't do anything if tw=0 as it should be. And you do not enable "t" (also
on by default), so you do not want to wrap text anyway? Confused now.
Segher
