On 09/18/2014 07:52 AM, Segher Boessenkool wrote:>> +# 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.
Right, I've forgot to fix it before sending the patch.
Too much experimenting in the evening...
> It is not marked as phony.
Noted.
> It should not _be_ phony; the two files should
> be separate targets.
I've done that initially but that may look weird for the user.
When typing 'make .local.vimrc' in GCC build directory one would expect
.local.vimrc to be created at the root of build directory, not srcdir.
> Why make links instead of copies? A user will likely
> want to edit his config.
I see your point. On the other hand fixing a bug in contrib/vimrc
will not be propagated to .local.vimrc which looks like a major disadvantage
(to me at least).
> The way you use ";" is wrong (it continues if there
> is an error).
Agreed. Current Makefiles do use ";" in backticks and that drew me away.
> You don't need the "cd" anyway, come to that.
Noted.
> 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.
I personally prefer a Makefile target to simplify things.
But let's wait for other people opinions on this.
>> --- /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.
How about adding a disclaimer? E.g. "beware that Vim plugins are a
GAPING SECURITY HOLE
so use the at YOUR OWN RISK". (And note that Braun's plugin does use
sandboxes).
> Or not mention it at all. Esp. since your next option
> has all the same functionality and more.
It lacks very important functionality: user has to specify path
to concrete GCC source tree when adding the autocmd.
I have a dozen of trees on my box and I regularly rename, move or copy them.
With plugins one doesn't have to bother fixing paths in ~/.vimrc
which is important for productivity.
>> +" 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_.
So "if you don't want to use plugins"?
>> +" 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 ;-)
Note that user has to type source command for every newly opened file.
This indeed looks inconvenient (to me again).
> Just keep things neutral please.
Trying to salt the boring docs a bit to attract reader's attention ;)
>> + 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.
IMHO matching shiftwidth with GNU indent may be useful.
E.g. Vim won't reindent when you start editing an empty line and user
will have to insert indents manually.
Also replacing offsets with numbers hides the fact
that they are based on GNU shiftwidth.
>> + setlocal textwidth=79
>
> The coding conventions say maximum line length is 80.
From https://www.gnu.org/prep/standards/html_node/Formatting.html :
"Please keep the length of source lines to 79 characters or less, for
maximum readability in the widest range of environments."
> 'tw' is a user preference as well
The config just follows the GNU coding standard. Now we rarely do
violate textwidth in our codes, that's why I do formatoptions+=l below.
>> + 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.
Me as well, the original config author did it that way. IMHO +t makes
sense here.
-Y
