[libvirt] [PATCH][RFC] Add a rule for indenting labels
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
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
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
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 :|
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
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
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Martin Kletzander