Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miscellaneous Unixext minor updates #6132

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

freddy77
Copy link
Collaborator

The majorities of changes are mainly spaces fixes.

Besides that, did some minor improvements and add a test to check changed code.

Use space instead of tabs for coherence with other part of the code.
Avoid spaces at the end of the line.

Signed-off-by: Frediano Ziglio <[email protected]>
Simplify code, avoid computing string length multiple times.

Signed-off-by: Frediano Ziglio <[email protected]>
Minor optimization, do the operation without engine lock.

Signed-off-by: Frediano Ziglio <[email protected]>
Very simple test.
Exercise modified code.

Signed-off-by: Frediano Ziglio <[email protected]>
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was the indentation changed? I would like to see this to be done with indent or some other formatter.

@freddy77
Copy link
Collaborator Author

How was the indentation changed? I would like to see this to be done with indent or some other formatter.

Manually. As far as I know, C files in Xapi do not have a proper indent configuration. Most of the changes are tabs to spaces using the file indentation size. Most of the lines are not using tabs. That makes the file coherent with themselves.
It seems that these sources came from various other projects, so potentially not all files have the same formattings, but at least every single file should be consistent with itself. Saying that a common indentation in Xapi could be an issue as syncing with other projects is made more complicated.

@lindig
Copy link
Contributor

lindig commented Nov 22, 2024

I think this should be the start of a formatting that we apply automatically; even if we do this one by one and not for all files today. But I see no reason to prefer one personal style over another.

@freddy77
Copy link
Collaborator Author

freddy77 commented Nov 25, 2024

@lindig @contificate I think a general format is a bit off the scope here. However, if we/you want this, we must be a bit more proactive. Is there any specific format you would like to use? Or it's just that any coherent format would be fine?

What about this: we use just a standard clang-format (as suggested) standard style, no other customizations. Which one? We format all C files with all possible standard styles (not many, and it can be automated with a script) and with a git diff (or git diff -w -b) we check which one has less line changes. NOTE: we don't have to indent all files at once, but the statistics to choose the better should be done considering them all.

@freddy77
Copy link
Collaborator Author

$ cat format 
#!/bin/bash

set -e

try() {
	local style="$1"
	echo "Style indent $style"
	git reset --hard origin/master &> /dev/null
	for fn in $(find -name \*.[ch]); do
		indent -$style $fn -o $fn.tmp
		mv -f $fn.tmp $fn
	done
	git diff origin/master | diffstat -f 0 | grep deletions
	git diff -w -b origin/master | diffstat -f 0 | grep deletions
	git reset --hard origin/master &> /dev/null
}

for style in gnu kr linux orig; do
	try $style
done

try() {
	local style="$1"
	echo "Style clang-format $style"
	git reset --hard origin/master &> /dev/null
	for fn in $(find -name \*.[ch]); do
		clang-format -style=$style -i $fn
	done
	git diff origin/master | diffstat -f 0 | grep deletions
	git diff -w -b origin/master | diffstat -f 0 | grep deletions
	git reset --hard origin/master &> /dev/null
}

for style in LLVM GNU Google Chromium Microsoft Mozilla WebKit; do
	try $style
done

$ ./format 
Style indent gnu
 30 files changed, 2959 insertions(+), 2818 deletions(-)
 23 files changed, 595 insertions(+), 454 deletions(-)
Style indent kr
 30 files changed, 2305 insertions(+), 2429 deletions(-)
 24 files changed, 506 insertions(+), 630 deletions(-)
Style indent linux
 30 files changed, 2840 insertions(+), 3003 deletions(-)
 24 files changed, 632 insertions(+), 795 deletions(-)
Style indent orig
 29 files changed, 2902 insertions(+), 2527 deletions(-)
 25 files changed, 1081 insertions(+), 706 deletions(-)
Style clang-format LLVM
 29 files changed, 2587 insertions(+), 3226 deletions(-)
 27 files changed, 801 insertions(+), 1440 deletions(-)
Style clang-format GNU
 30 files changed, 2840 insertions(+), 2906 deletions(-)
 27 files changed, 867 insertions(+), 933 deletions(-)
Style clang-format Google
 29 files changed, 2532 insertions(+), 3254 deletions(-)
 27 files changed, 862 insertions(+), 1584 deletions(-)
Style clang-format Chromium
 30 files changed, 2680 insertions(+), 3224 deletions(-)
 28 files changed, 911 insertions(+), 1455 deletions(-)
Style clang-format Microsoft
 29 files changed, 1619 insertions(+), 2096 deletions(-)
 27 files changed, 659 insertions(+), 1136 deletions(-)
Style clang-format Mozilla
 30 files changed, 2848 insertions(+), 3009 deletions(-)
 30 files changed, 965 insertions(+), 1126 deletions(-)
Style clang-format WebKit
 30 files changed, 1849 insertions(+), 2288 deletions(-)
 26 files changed, 440 insertions(+), 879 deletions(-)

Microsoft ?

@lindig
Copy link
Contributor

lindig commented Nov 25, 2024

I am not particular which style we use as long we pick one consistently. The stats are interesting but don't think that we have to pick the one with the smallest number of changes. @edwintorok said he has some style file.

@contificate
Copy link
Contributor

There is the GNU-derived styling @edwintorok used in this patch.

For C, I have preferences that go against the norm: so I'll excuse myself from any bikeshedding that could occur. In the end, it should just be consistent - no matter what it is.

@edwintorok
Copy link
Contributor

Here is the .clangformat in one of my other PRs: 24d650a
But yeah we could as well use GNU indent, I don't really mind, as long as it is consistent.

@edwintorok
Copy link
Contributor

Another possibility is this one (which adopts Linux style): edwintorok/xen@7ad754f#diff-7efcaaa7f3373d833201898fed73a80fb4be4b2bc6edebad63b587b4cbd9834f

Test.make ~count:1 ~name:__FUNCTION__ gen @@ fun _ ->
let fn = "test_file_direct" in
let buf = Bytes.make 10000 'x' in
Unixext.Direct.with_openfile fn [Unix.O_RDWR; Unix.O_CREAT] 0o600 (fun f ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan on using this module in a future PR?

There is a lot of dead code in XAPI unfortunately, and in this case the entire Unixext.Direct module appears to be dead code. If I drop the entire module from the interface and impl, then XAPI still builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optimizations and this testcase looks correct, but if this is dead code we could also simplify things and delete the unused C stubs, and modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants