| Message ID | 5419C01C.2040404@samsung.com |
|---|---|
| State | New |
| Headers | show |
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
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
On Sep 18, 2014, at 1:40 AM, Yury Gribov <y.gribov@samsung.com> wrote: > 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). Building gcc features a security risk at least as big as a plugin for vim. And, yes, I’ve built gcc in a sandbox before.
On Thu, Sep 18, 2014 at 12:40:08PM +0400, Yury Gribov wrote: > 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. Yes, you would not expect it to do anything to your source dir, ever :-) > >> +" 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). This *particular* plugin is suicidal. Most plugins are just fine. > > 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 was talking about mbr's plugin here :-) > 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. And :au bufread ~/src/gcc/* ... works for me. To each their own. > >> +" 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"? Just mention it as another option? Something like "You can add these options to your .vimrc; or you can :source this script file; or do either with an :autocmd; or use e.g. the <name of plugin here> plugin <some vim.org url>". Don't say "do X if Y"; let people decide for themselves what fits their situation best. > >> +" 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). Well for most people it is justt ":so contrib/vimrc". Or just ":lo" if you're talking about crazy people with views. > >> + 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. I have no idea what you mean with "matching with GNU indent", sorry. I was suggesting you could write it as :set cino=>4,n-2,{2,^-2,:2,=2,g0,f0,h2,p4,t0,+2,(0,u0,w1,m0 and you'd be independent of sw setting. The coding standard says "indent two spaces" etc. anyway. And yeah sw=2 does make sense for editing GCC, if you are used to sw=2 that is. The point is that the sw setting has nothing to do with what your text will look like, only with what keys you press. > >> + 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." There is a doc on gcc.gnu.org as well, which describes many more details. > Now we rarely do violate textwidth in our codes, "rarely"? Ho hum. There are many worse formatting errors, of course. And how much do those matter _really_. > > 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. It is certainly more consistent. Segher
On 09/18/2014 09:20 PM, Segher Boessenkool wrote: > On Thu, Sep 18, 2014 at 12:40:08PM +0400, Yury Gribov wrote: >> 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. > > Yes, you would not expect it to do anything to your source dir, ever :-) But that's already the case for some other GCC targets e.g. "make TAGS". So I'm just following an established practice here. >>> 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 was talking about mbr's plugin here :-) Ah, ok. Then I'll mention thinca's plugin as a secondary option with a disclaimer then. >>>> +" 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"? > > Just mention it as another option? Something like > "You can add these options to your .vimrc; or you can :source this script > file; or do either with an :autocmd; or use e.g. the <name of plugin here> > plugin <some vim.org url>". Don't say "do X if Y"; let people decide for > themselves what fits their situation best. Ok. >>>> +" 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). > > Well for most people it is justt ":so contrib/vimrc". Or just ":lo" if > you're talking about crazy people with views. For many people that would be a hassle (judging by my colleagues). > I was suggesting you could write it as > :set cino=>4,n-2,{2,^-2,:2,=2,g0,f0,h2,p4,t0,+2,(0,u0,w1,m0 > and you'd be independent of sw setting. > ... > And yeah sw=2 does make sense for editing GCC, if you are used to sw=2 > that is. The point is that the sw setting has nothing to do with what > your text will look like, only with what keys you press. Depending on whether you treat shiftwidth as "amount of spaces that is inserted when I press >" or "default indent for a particular class of files". For example with shiftwidth=2 user could (un)indent block of C code with < or > which seems to be useful. > The coding standard says > "indent two spaces" etc. anyway. That's what I meant by "matching with GNU indent". >>>> + 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." > > There is a doc on gcc.gnu.org as well, which describes many more details. Indeed: "Lines shall be at most 80 columns" (from https://gcc.gnu.org/codingconventions.html#Line). -Y
On Fri, Sep 19, 2014 at 08:17:28AM +0400, Yury Gribov wrote: > > I was talking about mbr's plugin here :-) > > Ah, ok. Then I'll mention thinca's plugin as a secondary option with a > disclaimer then. Why? There are more plugins that also do the same thing, all more popular (on vim.org at least), all less dangerous (well, many are probably just as bad :-( ). > > I was suggesting you could write it as > > :set cino=>4,n-2,{2,^-2,:2,=2,g0,f0,h2,p4,t0,+2,(0,u0,w1,m0 > > and you'd be independent of sw setting. > > ... > > And yeah sw=2 does make sense for editing GCC, if you are used to sw=2 > > that is. The point is that the sw setting has nothing to do with what > > your text will look like, only with what keys you press. > > Depending on whether you treat shiftwidth as "amount of spaces that is > inserted when I press >" or "default indent for a particular class of > files". For example with shiftwidth=2 user could (un)indent block of C > code with < or > which seems to be useful. Vim treats it as the former (it has no concept of the latter). cindent uses it too, for that "s" thing, but you do not have to use it here anyway since our coding standard requires two spaces so we can just hardcode that in cino; and in that case, a user can use any sw he wants / is used to. Using "s" in cino is useful if you want the layout to change if you change the shiftwidth. But we don't. Segher
commit fa7d6c68b0eb8763333aef6c22e4c88aa82db05e
Author: Yury Gribov <y.gribov@samsung.com>
Date: Thu Sep 4 16:55:44 2014 +0400
2014-09-17 Laurynas Biveinis <laurynas.biveinis@gmail.com>
Yury Gribov <y.gribov@samsung.com>
Vim config with GNU formatting.
contrib/
* vimrc: New file.
/
* .gitignore: Added .local.vimrc and .lvimrc.
* Makefile.tpl (.local.vimrc): New target.
* Makefile.in: Regenerate.
diff --git a/.gitignore b/.gitignore
index e9b56be..ab97ac6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -32,6 +32,9 @@ POTFILES
TAGS
TAGS.sub
+.local.vimrc
+.lvimrc
+
.gdbinit
.gdb_history
diff --git a/Makefile.in b/Makefile.in
index d6105b3..1be75b7 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -2384,6 +2384,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)
+
# Installation targets.
.PHONY: install uninstall
diff --git a/Makefile.tpl b/Makefile.tpl
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)
+
# Installation targets.
.PHONY: install uninstall
diff --git a/contrib/vimrc b/contrib/vimrc
new file mode 100644
index 0000000..88e77a6
--- /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
+" Or install Markus Braun's localvimrc and do
+" $ make .lvimrc
+" Or if you dislike plugins, add autocmd in your ~/.vimrc:
+" :au BufNewFile,BufReadPost path/to/gcc/* :so path/to/gcc/contrib/vimrc
+" Or just source file manually every time if you are masochist:
+" :so path/to/gcc/contrib/vimrc
+"
+" Copyright (C) 2014 Free Software Foundation, Inc.
+"
+" This program is free software; you can redistribute it and/or modify
+" it under the terms of the GNU General Public License as published by
+" the Free Software Foundation; either version 3 of the License, or
+" (at your option) any later version.
+"
+" This program is distributed in the hope that it will be useful,
+" but WITHOUT ANY WARRANTY; without even the implied warranty of
+" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+" GNU General Public License for more details.
+"
+" You should have received a copy of the GNU General Public License
+" along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+function! SetStyle()
+ let l:fname = expand("%:p")
+ if stridx(l:fname, 'libsanitizer') != -1
+ return
+ endif
+ let l:ext = fnamemodify(l:fname, ":e")
+ let l:c_exts = ['c', 'h', 'cpp', 'cc', 'C', 'H', 'def', 'java']
+ if index(l:c_exts, l:ext) != -1
+ 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
+ setlocal textwidth=79
+ setlocal formatoptions-=ro formatoptions+=cql
+ endif
+endfunction
+
+call SetStyle()