| Message ID | 81d621cea5975fdd9698992b968dcd7528c637af.1671214525.git.edwin.torok@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OCaml fixes | expand |
> On 16 Dec 2022, at 18:25, Edwin Török <edvin.torok@citrix.com> wrote: > > See CODING_STYLE: Xen uses spaces, not tabs. > > * OCaml code: > > Using `ocp-indent` for now to just make minimal modifications in > tabs vs spaces and get the right indentation. > We can introduce `ocamlformat` later. > > * C stubs: > > just replace tabs with spaces now, using `indent` or `clang-format` > would change code too much for 4.17. > > This avoids perpetuating a formatting style that is inconsistent with > the rest of Xen, and that makes preparing and submitting patches more > difficult (OCaml indentation tools usually only support spaces, not tabs). > > No functional change. > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> Acked-by: Christian Lindig <christian.lindig@citrix.com> I am in favour of automating the code formatting, and moving to OCamlformat in particular. The main drawback is that it can make back porting more difficult because formatting can change not just indentation but placement of parentheses and similar grouping syntax as well, leading to changes between the current code and older code that have to me resolved manually. — C
On Fri, Dec 16, 2022 at 06:25:12PM +0000, Edwin Török wrote:
> + git ls-files '*.c' '*.h' | xargs -n1 sed -ie 's/\t/ /g'
Seen as there's a patch adding .clang-format, what the point of this
command?
"-ie" means to ask sed to change file in-place an keep a copy of the
original file with "e" as suffix. So please replace that by "-i~" or
"-i -e" or just "-i", with the first one create a "~" backup, the last
two not creating a backup at all and probably what you wanted. ("-e" is
optional as there's only a single command)
Thanks,
> On 19 Dec 2022, at 11:52, Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Fri, Dec 16, 2022 at 06:25:12PM +0000, Edwin Török wrote: >> + git ls-files '*.c' '*.h' | xargs -n1 sed -ie 's/\t/ /g' > > Seen as there's a patch adding .clang-format, what the point of this > command? The diff to change tabs to spaces (and the equivalent one from ocp-indent) can be proven to introduce 0 changes by looking at the diff with ignore-whitespace. Proving the same with ocamlformat or clang-format is more difficult (and in particular if you keep rebasing commits from after the reformat commit to before it you risk losing the change if you don't redo the format commit correctly). So I intended to do this gradually: first get indentation to be consistent, and then get formatting to be consistent later once the former has been accepted and committed. > > "-ie" means to ask sed to change file in-place an keep a copy of the > original file with "e" as suffix. So please replace that by "-i~" or > "-i -e" or just "-i", with the first one create a "~" backup, the last > two not creating a backup at all and probably what you wanted. ("-e" is > optional as there's only a single command) > Thanks for pointing it out, what I wanted is 'sed -i -e', and you can usually merge multiple single char flags into a single one, except that is not the case for -i, and I keep making this mistake. .PHONY: format format: git ls-files '*.ml' '*.mli' | xargs -n1 ocp-indent -i git ls-files '*.c' '*.h' | xargs -n1 sed -i 's/\t/ /g' --Edwin
diff --git a/tools/ocaml/Makefile b/tools/ocaml/Makefile index a7c04b6546..274ba15d75 100644 --- a/tools/ocaml/Makefile +++ b/tools/ocaml/Makefile @@ -34,3 +34,8 @@ build-tools-oxenstored: $(MAKE) -s -C libs/xb $(MAKE) -s -C libs/xc $(MAKE) -C xenstored + +.PHONY: format +format: + git ls-files '*.ml' '*.mli' | xargs -n1 ocp-indent -i + git ls-files '*.c' '*.h' | xargs -n1 sed -ie 's/\t/ /g'
See CODING_STYLE: Xen uses spaces, not tabs. * OCaml code: Using `ocp-indent` for now to just make minimal modifications in tabs vs spaces and get the right indentation. We can introduce `ocamlformat` later. * C stubs: just replace tabs with spaces now, using `indent` or `clang-format` would change code too much for 4.17. This avoids perpetuating a formatting style that is inconsistent with the rest of Xen, and that makes preparing and submitting patches more difficult (OCaml indentation tools usually only support spaces, not tabs). No functional change. Signed-off-by: Edwin Török <edvin.torok@citrix.com> -- Reason for inclusion: - avoid perpetuating a different coding style (I thought tabs were mandated by Xen, and was about to fix up my editor config to match when I realized Xen already mandates the use of spaces) - should make submitting patches for OCaml easier (OCaml indentation tools know only about spaces, so I either can't use them, or have to manually adjust indentation every time I submit a patch) - it can be verified that the only change here is the Makefile change for the new rule, 'git log -p -1 -w' should be otherwise empty Changes since v3: - this didn't make it into 4.17.0, we'll reconsider for 4.17.1, for now apply just to master which is open again - separate introducing the rule from actual reformatting Cc: Christian Lindig <christian.lindig@citrix.com> --- tools/ocaml/Makefile | 5 +++++ 1 file changed, 5 insertions(+)