Adopt style(9) using clang-format #55
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fel1x-developer
force-pushed
the
clang-format
branch
3 times, most recently
from
d0a2168 to
a66c981
Compare
ngie-eign
requested changes
Feb 22, 2024
There was a problem hiding this comment.
@fel1x-developer : for the sake of pragmatism, I think it's best to apply this change on a per-file basis since this is going to cause a lot of churn for little to no immediate benefit.
Also, there are a number of conflicts that need to be resolved in this change.
fel1x-developer
force-pushed
the
clang-format
branch
from
a66c981 to
b9afd94
Compare
fel1x-developer
changed the title
fel1x-developer
force-pushed
the
clang-format
branch
from
b9afd94 to
5c63449
Compare
|
Was this pulled from FreeBSD? If so, it might be better to figure out a way to keep this file in sync, somehow, instead of it drifting too far from upstream (freebsd-src). |
For now, automatically tracking updates in the freebse-src upstream is impossible. We can use post-commit hook as an alternative. By the way, the |
fel1x-developer
force-pushed
the
clang-format
branch
from
5c63449 to
fcbb60f
Compare
|
Could you please add a comment to the file to note where it came from (FreeBSD; git revision <...>)? Thanks :). |
fel1x-developer
force-pushed
the
clang-format
branch
from
fcbb60f to
8e7eab5
Compare
This allows repeatedly polling some command until it succeeds, to some timeout limit.
Sponsored by: The FreeBSD Foundation
`expr ${OPTIND} - 1` can be replaced with $((OPTIND - 1)) to avoid spawning
a new processes. While it's unlikely to make much of a performance
difference for a single execution, it happens once for ever test case so
it will add up for a full testsuite run.
I have been trying to speed up the time it takes to run the testsuite for
CheriBSD on QEMU RISC-V (originally 22 hours). One of the slowest tests is
the pfctl test since ever single invocation of it spends ~90 seconds
generating the list of available tests (even when running only one test).
And this is after the change in r365708 that removes ~200 calls to
atf_get_srcdir(). Looking at truss output it turns out that using atf-sh
results in many tr processes being spawned to perform string substitution.
The _atf_normalize() function tries to remove characters that are not valid
in shell variable names by replacing . and - with _. However, most strings
passed to the function don't contain those characters, so we unnecessarily
fork() a new tr processes (which adds up to a significant slowdown).
With this change we only call tr if the variable contains one of the
illegal characters rather than unconditionally.
Ideally we would do this substitution using only shell builtins, but
unfortunately the string substitution syntax ${var//} is not supported by
POSIX sh and I am not aware of any alternatives.
Basic time measurements on CHERI-QEMU RISC-V usng
`/usr/bin/time /usr/tests/sbin/pfctl_test -l > /dev/null`:
Before:
`90.68 real 89.99 user 0.10 sys`
After:
`14.50 real 14.31 user 0.10 sys`
I.e. 85% of the time was being spent in these tr invocations!
truss -cf before:
```
syscall seconds calls errors
fork 453.330532000 2371 0
cap_enter 0.247944000 633 0
cap_ioctls_limit 0.761099000 1899 0
vfork 0.421971000 2 0
getegid 0.000342000 1 0
getgid 0.000358000 1 0
getuid 0.000324000 1 0
getppid 0.000589000 1 0
geteuid 0.000631000 2 0
getpid 0.000328000 1 0
issetugid 0.450306000 1270 0
write 0.415779000 634 0
__sysctl 0.280936000 634 0
sigprocmask 2.543827000 6340 0
readlink 0.317952000 635 635
read 1.319873000 2532 0
pread 0.327678000 634 0
open 1.399164000 2536 634
munmap 0.301268000 634 0
mprotect 0.566142000 1268 0
mmap 7.117084000 8876 0
fstat 1.326981000 3168 0
dup2 0.954023000 2373 0
close 4.496506000 9979 0
clock_gettime 0.740010000 1902 0
cap_rights_limit 0.770141000 1899 0
cap_fcntls_limit 0.695135000 1899 0
------------- ------- -------
478.786923000 52125 1269
```
After:
```
syscall seconds calls errors
fork 24.675445000 1107 0
cap_enter 0.000340000 1 0
cap_ioctls_limit 0.001009000 3 0
vfork 0.228345000 1 0
getegid 0.000318000 1 0
getgid 0.000323000 1 0
getuid 0.000345000 1 0
getppid 0.000329000 1 0
geteuid 0.000726000 2 0
getpid 0.000335000 1 0
issetugid 0.001554000 4 0
write 0.000539000 1 0
__sysctl 0.000369000 1 0
sigprocmask 0.003684000 10 0
readlink 0.000440000 1 1
read 0.001318000 2 0
pread 0.000556000 1 0
open 0.002250000 4 1
munmap 0.000524000 1 0
mprotect 0.000767000 2 0
mmap 0.011109000 14 0
fstat 0.001743000 4 0
dup2 0.398638000 1108 0
close 1.996684000 4916 0
clock_gettime 0.001047000 3 0
cap_rights_limit 0.001170000 3 0
cap_fcntls_limit 0.000930000 3 0
------------- ------- -------
27.330837000 7197 2
```
atf-sh/atf-check.1[126]: Sentence does not start on new line
Also create a junit xml file.
CirrusCI doesn't like iso-8859-1, hopefully it accepts utf-8. The test outputs should only contain ASCII characters so this shouldn't matter.
Signed-off-by: Kyle Evans <[email protected]>
There are more of these that need to be modernized, but this will do for now. Signed-off-by: Kyle Evans <[email protected]>
We will be reusing this when we later allow the results file to be closed and reopened for internal test reasons, so do this cleanup in advance. Signed-off-by: Kyle Evans <[email protected]>
Don't bother with different logic based on ctx->resfile, just set ctx->resfilefd up initially with STDOUT_FILENO/STDERR_FILENO and write to the ctx->resfilefd unconditionally later. Signed-off-by: Kyle Evans <[email protected]>
This allows us to set the resultsfile mid-run if we need to, as will be the case for some of our atf internal tests. Notably, some internal tests will fork and do ATF_REQUIRE's that we do -not- want to be written to our outer-test's results file. Signed-off-by: Kyle Evans <[email protected]>
This will be needed for some internal tests that currently fail as they invoke some inner-test that's supposed to fail, but it isn't supposed to write to the outer test's result file. Signed-off-by: Kyle Evans <[email protected]>
Signed-off-by: Kyle Evans <[email protected]>
…d. relax the const constraints to allow the array to be freed.
Release 0.21 was released in Oct 2014.
The example saved the output to `stdout`. Test `stdout` instead of the nonexistent file, `ls` Co-authored-by: Enji Cooper <[email protected]>
Fix typo in atf-sh.3
Some of the compiler versions I tested for my work spotted that the path ret == -1 && errno != ENOENT does not return a value.
Cover all paths
The previous logic used 2 separate calls to `atf::fs::path::str()` when constructing a `std::vector<char>` to pass to `mkstemp(..)`. This in turn caused grief with how data copying is done in atf-c(3), etc, as the prior code computed the length of the path of an internal buffer in `atf_dynstr` structs. Moreover, the code was manually appending a nul char, which was unnecessary when making the valid assumption that `std::string` is a nul-terminated string. The new code convert the path to an `std::string` once, includes the existing nul char in the buffer, then passes it to mkstemp(3) instead. The code works properly now. Closes: freebsd#76 Signed-off-by: Enji Cooper <[email protected]>
atf_check: fix std::length_error thrown from temp_file
- atf_utils_cat_file: output `strerror(errno)` to help diagnose why the `open` call failed. - atf_utils_grep_file: output a message with the path name and `strerror(errno)` for parity with `atf_utils_cat_file(..)` and to make diagnosing problems without a corefile possible. Signed-off-by: Enji Cooper <[email protected]>
This is being done prior to releasing 0.22.
Signed-off-by: Enji Cooper <[email protected]>
Update the version to 0.23
atf::fs::path has a str() method, and it returns a std::string constructor. I found this odd that a vector is used here to copy std::string contents. As std::string is a typedef of std::basic_string<char>, this additional copy is not needed. Also, remove calling the str() method on error. Signed-off-by: rilysh <[email protected]>
atf-check.cpp: remove unneeded copying into vector
Improve diagnostics when paths cannot be opened
Revert "atf-check.cpp: remove unneeded copying into vector"
As noted by the reporter, the "descr" metadata property was made optional back in the 0.8 release (af4059b to be specific). Update the documentation posthumously per the code change. Closes: freebsd#63 Reported by: Roland Illig (@rillig) Signed-off-by: Enji Cooper <[email protected]>
Mark the "descr" metadata property "Optional"
* admin/make-release.sh: automate release creation This does the absolute bare minimum to produce a release. This logic best resembles the expectations for legacy (pre-0.22) releases of ATF. Various files are omitted as they are not required in order to build from a full `autoreconf`'ed source distribution. Signed-off-by: Enji Cooper <[email protected]>
atf::fs::path has a str() method, and it returns a std::string constructor. I found this odd that a vector is used here to copy std::string contents. As std::string is a typedef of std::basic_string<char>, this additional copy is not needed. Also, remove calling the str() method on error. Notes by ngie@: `buf.data()` returns `const char*` until C++17, so it can't be used in combination with `mkstemp(..)` (it mutates the template argument). Access the internal buffer with the `operator[]` instead. Add the C++17+ form as well along with a TODO comment, so the code can move to std::string::data eventually. Co-authored by: Enji Cooper <[email protected]> Signed-off-by: rilysh <[email protected]> Co-authored-by: rilysh <[email protected]>
fel1x-developer
force-pushed
the
clang-format
branch
from
8e7eab5 to
9dfdf72
Compare
This commit adopts FreeBSD's style(9). This file is retrieved from https://github.com/freebsd/freebsd-src/blob/main/.clang-format. Contributers can apply this style on modified files in future changes. Signed-off-by: Minsoo Choo <[email protected]>
fel1x-developer
force-pushed
the
clang-format
branch
from
9dfdf72 to
b1e4ae7
Compare
|
@ngie-eign Sorry this PR was mixed with other unrelated changes I was working on. We only need 1 commit where we bring We might need a git hook so people can format files according to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I find atk's coding style doesn't follow some conventions, such as having space in
<>or putting pointer*or reference symbol&with variable types, not variable names.FreeBSD's coding style style(9) can be applied here. One main exception in style(9) opposed to the "convention" is that style(9) use 8-space indentation while most projects use 4-space indentation. (2-space indentation is predominant in web dev). We can discuss whether or not to use 8-space indentation if needed.