-
Notifications
You must be signed in to change notification settings - Fork 285
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
base: master
Are you sure you want to change the base?
Conversation
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]>
There was a problem hiding this 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.
Manually. As far as I know, C files in Xapi do not have a proper |
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. |
@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 |
$ 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(-)
|
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. |
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. |
Here is the .clangformat in one of my other PRs: 24d650a |
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 -> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The majorities of changes are mainly spaces fixes.
Besides that, did some minor improvements and add a test to check changed code.