8:11 a.m.
Indent labels by one space.
Add the rule to HACKING and enforce it by syntax-check.
---
Adding a space helps git's function context detection, but the fallout
patch is over 800K.
My reasoning for also indenting labels in nested code is that
vim's cinoptions don't have a separate option for these labels
(or I haven't searched hard enough?).
Inspired by this thread:
https://www.redhat.com/archives/libvir-list/2014-March/msg01310.html
cfg.mk | 7 +++++++
docs/hacking.html.in | 18 +++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk
index 559f719..1512fb2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -898,6 +898,13 @@ sc_prohibit_virConnectOpen_in_virsh:
halt='Use vshConnect() in virsh instead of virConnectOpen*' \
$(_sc_search_regexp)
+sc_require_space_before_label:
+ @prohibit='^(( {2})*|( {3})*)([_a-zA-Z0-9]+):$$' \
+ exclude='( {4})*default:$$' \
+ in_vc_files='\.[ch]$$' \
+ halt="Labels should be indented by 4*n + 1 spaces" \
+ $(_sc_search_regexp)
+
sc_curly_braces_style:
@files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \
$(GREP) -nHP \
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 0febee2..b2b158a 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -303,7 +303,7 @@
set tabstop=8
set shiftwidth=4
set expandtab
- set cinoptions=(0,:0,l1,t0
+ set cinoptions=(0,:0,l1,t0,L3
filetype plugin indent on
au FileType make setlocal noexpandtab
au BufRead,BufNewFile *.am setlocal noexpandtab
@@ -1139,6 +1139,22 @@
retry: If needing to jump upwards (e.g., retry on EINTR)
</pre>
+ <p>
+ Labels should be indented by one space (putting them on the beginning
+ of the line confuses function context detection in emacs and git):
+ </p>
+
+<pre>
+int foo()
+{
+ if (wizz) {
+ retry:
+ goto retry;
+ }
+ cleanup:
+}
+</pre>
+
<h2><a name="committers">Libvirt committer
guidelines</a></h2>
--
1.8.3.2
9:04 a.m.
On Fri, Mar 21, 2014 at 02:11:10PM +0100, Ján Tomko wrote:
Indent labels by one space.
Oooh yes, this is something I wanted, but didn't have the guts to do
it...
Add the rule to HACKING and enforce it by syntax-check.
---
Adding a space helps git's function context detection, but the fallout
patch is over 800K.
... basically because of this *and* because of many people having some
defaults which would then indent incorrectly. I'm guessing vim has
indentation of labels set to no indent (or whatever is the
configuration there) by default, doesn't it? But I see you added a
configuration for that, so at least that's OK.
My reasoning for also indenting labels in nested code is that
vim's cinoptions don't have a separate option for these labels
(or I haven't searched hard enough?).
So that's why you went with 4n+1?
Inspired by this thread:
https://www.redhat.com/archives/libvir-list/2014-March/msg01310.html
cfg.mk | 7 +++++++
docs/hacking.html.in | 18 +++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk
index 559f719..1512fb2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -898,6 +898,13 @@ sc_prohibit_virConnectOpen_in_virsh:
halt='Use vshConnect() in virsh instead of virConnectOpen*' \
$(_sc_search_regexp)
+sc_require_space_before_label:
+ @prohibit='^(( {2})*|( {3})*)([_a-zA-Z0-9]+):$$' \
Doesn't this regexp allow only number of spaces not divisible by 2 or
3? That would report proper label in 2nd nesting level (if we have
such labels).
+ exclude='( {4})*default:$$'
\
+ in_vc_files='\.[ch]$$' \
+ halt="Labels should be indented by 4*n + 1 spaces" \
+ $(_sc_search_regexp)
+
sc_curly_braces_style:
@files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \
$(GREP) -nHP \
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 0febee2..b2b158a 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -303,7 +303,7 @@
set tabstop=8
set shiftwidth=4
set expandtab
- set cinoptions=(0,:0,l1,t0
+ set cinoptions=(0,:0,l1,t0,L3
Could you modify the emacs defaults as well?
filetype plugin indent on
au FileType make setlocal noexpandtab
au BufRead,BufNewFile *.am setlocal noexpandtab
@@ -1139,6 +1139,22 @@
retry: If needing to jump upwards (e.g., retry on EINTR)
</pre>
+ <p>
+ Labels should be indented by one space (putting them on the beginning
+ of the line confuses function context detection in emacs and git):
It confuses only git, emacs just indents it differently by default.
+ </p>
+
+<pre>
+int foo()
+{
+ if (wizz) {
+ retry:
+ goto retry;
+ }
+ cleanup:
+}
+</pre>
+
<h2><a name="committers">Libvirt committer
guidelines</a></h2>
--
All in all, I'm for it, I just don't think others will like it...
Martin
9:19 a.m.
On 03/21/2014 07:11 AM, Ján Tomko wrote:
Indent labels by one space.
Add the rule to HACKING and enforce it by syntax-check.
---
Adding a space helps git's function context detection, but the fallout
patch is over 800K.
My reasoning for also indenting labels in nested code is that
vim's cinoptions don't have a separate option for these labels
(or I haven't searched hard enough?).
Alas, I don't know enough on vim to help there.
Inspired by this thread:
https://www.redhat.com/archives/libvir-list/2014-March/msg01310.html
cfg.mk | 7 +++++++
docs/hacking.html.in | 18 +++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk
index 559f719..1512fb2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -898,6 +898,13 @@ sc_prohibit_virConnectOpen_in_virsh:
halt='Use vshConnect() in virsh instead of virConnectOpen*' \
$(_sc_search_regexp)
+sc_require_space_before_label:
+ @prohibit='^(( {2})*|( {3})*)([_a-zA-Z0-9]+):$$' \
This doesn't do what you want; it sounds like you want to require all
labels to have (4n+1) spaces, so you want 4n+{0,2,3} to be forbidden.
Something like:
@prohibit='^( {4})*(| | )([_a-zA-Z0-9]+):$$'
+ exclude='( {4})*default:$$'
\
+ in_vc_files='\.[ch]$$' \
+ halt="Labels should be indented by 4*n + 1 spaces" \
+ $(_sc_search_regexp)
The rest of this makes sense, if I can figure out why my emacs settings
aren't doing this already, and if we update .dir-locals.el in the same
patch.
+<pre>
+int foo()
+{
+ if (wizz) {
+ retry:
+ goto retry;
+ }
+ cleanup:
+}
Wouldn't actually compile (a label needs to be attached to a statement,
and our warnings coupled with -Werror would call out the fact that
cleanup: is unused), but gets the point across.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:49 a.m.
On Fri, Mar 21, 2014 at 08:19:42AM -0600, Eric Blake wrote:
On 03/21/2014 07:11 AM, Ján Tomko wrote:
> Indent labels by one space.
>
> Add the rule to HACKING and enforce it by syntax-check.
> ---
> Adding a space helps git's function context detection, but the fallout
> patch is over 800K.
>
> My reasoning for also indenting labels in nested code is that
> vim's cinoptions don't have a separate option for these labels
> (or I haven't searched hard enough?).
Alas, I don't know enough on vim to help there.
>
> Inspired by this thread:
> https://www.redhat.com/archives/libvir-list/2014-March/msg01310.html
>
> cfg.mk | 7 +++++++
> docs/hacking.html.in | 18 +++++++++++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index 559f719..1512fb2 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -898,6 +898,13 @@ sc_prohibit_virConnectOpen_in_virsh:
> halt='Use vshConnect() in virsh instead of virConnectOpen*' \
> $(_sc_search_regexp)
>
> +sc_require_space_before_label:
> + @prohibit='^(( {2})*|( {3})*)([_a-zA-Z0-9]+):$$' \
This doesn't do what you want; it sounds like you want to require all
labels to have (4n+1) spaces, so you want 4n+{0,2,3} to be forbidden.
Something like:
@prohibit='^( {4})*(| | )([_a-zA-Z0-9]+):$$'
> + exclude='( {4})*default:$$' \
> + in_vc_files='\.[ch]$$' \
> + halt="Labels should be indented by 4*n + 1 spaces" \
> + $(_sc_search_regexp)
The rest of this makes sense, if I can figure out why my emacs settings
aren't doing this already, and if we update .dir-locals.el in the same
patch.
FWIW, my emacs is doing the right thing wrt this proposed hacking
rule and I don't have any .emacsrc file at all on this machine - just
plain default settings.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:13 p.m.
On 03/21/2014 08:49 AM, Daniel P. Berrange wrote:
On Fri, Mar 21, 2014 at 08:19:42AM -0600, Eric Blake wrote:
> On 03/21/2014 07:11 AM, Ján Tomko wrote:
>> Indent labels by one space.
>>
>> Add the rule to HACKING and enforce it by syntax-check.
>> ---
>> Adding a space helps git's function context detection, but the fallout
>> patch is over 800K.
>>
>> My reasoning for also indenting labels in nested code is that
>> vim's cinoptions don't have a separate option for these labels
>> (or I haven't searched hard enough?).
>
> Alas, I don't know enough on vim to help there.
Okay, I've spent some more time on the emacs side; emacs has the option
of a c-label-minimum-indent, but applies it only to top-level labels
(and not to nested code labels). Where it gets weird is that
c-label-minimum-indent is applied by default only for gnu mode indent,
but not other modes. We want libvirt in K&R mode indent. Emacs is
customizable, so you can request other indent modes to also use label
minimum indent by setting c-special-indent-hook, but as a hook it is
deemed dangerous when set via .dir-locals.el and warns the user on every
file that you open, making that particular customization harder to set
as a project-level default.
> This doesn't do what you want; it sounds like you want to
require all
> labels to have (4n+1) spaces, so you want 4n+{0,2,3} to be forbidden.
Emacs does not have an easy way to enforce (4n+1) spaces, only a minimum
indent. Furthermore, it is only the top-level labels that interfere
with git function locations; so at this point, I'm leaning towards a simpler
@prohibit='^[_a-zA-Z0-9]+):$$'
and just not worry about nested indent (we have fewer nested labels anyway).
> The rest of this makes sense, if I can figure out why my emacs
settings
> aren't doing this already, and if we update .dir-locals.el in the same
> patch.
FWIW, my emacs is doing the right thing wrt this proposed hacking
rule and I don't have any .emacsrc file at all on this machine - just
plain default settings.
I found the difference between Dan's setup and mine - in my ~/.emacs, I
actually created a new style mode libvirt-c-mode (I created it back in
2010, with Jim Meyering's help, before we had .dir-locals.el). Although
my mode and the settings in .dir-locals.el appear identical [basically:
(defun libvirt-c-mode ()
"C mode with adjusted defaults for use with libvirt."
(interactive)
(c-set-style "K&R")
(setq indent-tabs-mode nil) ; indent using spaces, not TABs
(setq c-indent-level 4)
(setq c-basic-offset 4))
(add-hook 'c-mode-hook
'(lambda () (if (string-match "/libvirt" (buffer-file-name))
(libvirt-c-mode))))
], where it gets different is that c-label-minimum-indent defaults to
only applying to gnu style. In creating my own style, I left gnu style,
so the minimum no longer applied to me. But adding:
(setq c-special-indent-hook c-gnu-impose-minimum) in the list of
settings in my mode fixed things so it behaves the way Dan was seeing;
as did completely removing my custom mode and instead relying on
.dir-locals.el. c-special-indent-hook won't work in .dir-locals.el, but
now that I know the cause of the difference and how to work around it on
my end, I'm okay if we force a minimum indent.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:23 p.m.
On 03/21/2014 11:13 AM, Eric Blake wrote:
as did completely removing my custom mode and instead relying on
.dir-locals.el. c-special-indent-hook won't work in .dir-locals.el,
More to the point, I recommend including this when you respin the patch:
diff --git i/docs/hacking.html.in w/docs/hacking.html.in
index 0febee2..61eebf5 100644
--- i/docs/hacking.html.in
+++ w/docs/hacking.html.in
@@ -274,25 +274,9 @@
indentation level, and other than that, follow the K&R style.
</p>
<p>
- If you use Emacs, add the following to one of one of your
start-up files
- (e.g., ~/.emacs), to help ensure that you get indentation right:
- </p>
-<pre>
- ;;; When editing C sources in libvirt, use this style.
- (defun libvirt-c-mode ()
- "C mode with adjusted defaults for use with libvirt."
- (interactive)
- (c-set-style "K&R")
- (setq indent-tabs-mode nil) ; indent using spaces, not TABs
- (setq c-indent-level 4)
- (setq c-basic-offset 4))
- (add-hook 'c-mode-hook
- '(lambda () (if (string-match "/libvirt" (buffer-file-name))
- (libvirt-c-mode))))
-</pre>
-
- <p>
- If you use vim, append the following to your ~/.vimrc file:
+ If you use Emacs, the project includes a file .dir-locals.el
+ that sets up the preferred indentation. If you use vim, append
+ the following to your ~/.vimrc file:
</p>
<pre>
set nocompatible
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3938
days inactive
3938
days old
5 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Martin Kletzander