skypjack
UVW v3
A breaking change to set a tipping point for a new version of uvw, an updated coding style (with a more standard-ish snake style) and huge performance improvements during compilation. Feedback are highly appreciated. π
Codecov Report
Merging #263 (c1e11a8) into master (b2ed37f) will increase coverage by
0.82%. The diff coverage is97.01%.
:mega: This organization is not using Codecovβs GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 96.44% 97.27% +0.82%
==========================================
Files 62 61 -1
Lines 2590 2420 -170
==========================================
- Hits 2498 2354 -144
+ Misses 92 66 -26
| Impacted Files | Coverage Ξ | |
|---|---|---|
| src/uvw/poll.cpp | 0.00% <0.00%> (ΓΈ) |
|
| src/uvw/loop.h | 60.00% <69.23%> (+30.00%) |
:arrow_up: |
| src/uvw/signal.cpp | 66.66% <71.42%> (ΓΈ) |
|
| src/uvw/stream.cpp | 84.61% <84.61%> (-15.39%) |
:arrow_down: |
| src/uvw/util.cpp | 86.58% <85.45%> (-3.93%) |
:arrow_down: |
| src/uvw/fs.h | 85.71% <85.71%> (-5.96%) |
:arrow_down: |
| src/uvw/tty.cpp | 87.50% <85.71%> (ΓΈ) |
|
| src/uvw/fs_event.cpp | 92.85% <92.30%> (-1.27%) |
:arrow_down: |
| src/uvw/udp.cpp | 93.61% <92.85%> (-1.74%) |
:arrow_down: |
| src/uvw/stream.h | 93.93% <94.33%> (-1.30%) |
:arrow_down: |
| ... and 57 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@skypjack do you need any help to get the pipeline back to green too?
Oh, well, put aside some sporadic failures mainly due to the poll tests, it depends on the reduced test coverage and I'm trying to increase it. Any help is appreciated ofc. π
I really like the reduced compile time and find the multi_word_style much better than multiWordStyle for readers.
(not) just joking:
Optimal would be to have a programming language that allows spaces in identifiers and ideal if we had enough words to only ever use single word identifiers ... but these are topics left as exercises for later generations (and languages).
@sthagen have you tried this branch already? I'm observing a huge improvement in terms of compilation time and I'm curious to know if it's a common thing. Also, feel free to drop comments, suggestions and complaints if you want. Any contribution is appreciated! π
@skypjack I did not, but I hope to do it on the weekend (... there are traces of such "plans of mine" here on github where a day became a month ... π€·π½ββοΈ) I read through the hundred or so files in the change set and in theory it really looks good. Practice may decide differently on my machine, but I am confident and happy your machines already show the practical gains π
Sometimes when many functions change their return type from void to int (like in this PR) change sets risk to become a liability. But, typically in wrappers / proxies like this library, this only indicates the beneficial move from a hiding facade pattern to an open proxy that propagates the state info from the lower layers back to the caller instead of providing riddles to the callers :+1:
Yeah, the idea is to make it more transparent and only add the work around memory and lifetime management on top of libuv. It doesn't make much sense to hide to the final user the error codes or any other thing, as if there wasn't libuv under the hood. π
@skypjack it fails tests on my env (WSL2 ubuntu-20.04):
[ctest] ../test/uvw/loop.cpp:86: Failure
[ctest] Expected equality of these values:
[ctest] count
[ctest] Which is: 11
[ctest] 12u
[ctest] Which is: 12
Mmm any chance you can reproduce it? I don't know what's going on there otherwise. π
Mmm any chance you can reproduce it? I don't know what's going on there otherwise. π
@stefanofiorentino ping π I'd like to merge this one upstream but I can't reproduce nor fix the failure you mentioned. Any news about it?
Some (little) investigations yield this:
- the failing test is Loop.walk
- by enabling USE_ASAN the "Loop" tests pass, but "616 byte(s) leaked in 5 allocation(s)".
I'm wondering, why we don't have USE_*SAN as github workflows?
I'm wondering, why we don't have USE_*SAN as github workflows?
Testing a workflow for that on this branch. π
Cool @stefanofiorentino at least I can reproduce the error on the CI. Good catch. π
@skypjack launching them 1-by-1 with --gtest_filter only Loop.Close causes the leak.
Probably the uv_loop_close returns UV_EBUSY. I've to stop investigations for a while.
Oh, wait, I suspect the test is wrong actually. We're closing the handle after the loop. π€¦ββοΈ Gotta go now but I'll give it a try during the weekend if possible. Thanks for checking it. β€οΈ
I see the workflows here are green, but my local build (after a git force-clean) still complains:
test$ ./loop --gtest_list_tests | grep "^ " | sed 's/^ //g' | xargs -r -n1 -I '{}' bash -c "./loop --gtest_filter=*{} 2&>1 > /dev/null || echo {} test is KO" Walk test is KO
What error do you get @stefanofiorentino
Loop.walk, a wrong counter?
Yes, my cmake conf is
"cmake.configureArgs": [
"-DBUILD_TESTING:BOOL=TRUE"
]
and the error is still
[ctest] ../test/uvw/loop.cpp:88: Failure
[ctest] Expected equality of these values:
[ctest] count
[ctest] Which is: 11
[ctest] 12u
[ctest] Which is: 12
Yeah @stefanofiorentino that's this line:
loop->resource<uvw::tty_handle>(0, true);
If you run it from the command line, it causes your error. If you can confirm it, we can also drop the line from the test. π
Yeah @stefanofiorentino that's this line:
loop->resource<uvw::tty_handle>(0, true);If you run it from the command line, it causes your error. If you can confirm it, we can also drop the line from the test. π
Ok, as setsid sh -c "./test/loop" is working for me.
I'm going to approve this ;)
Any other concerns? I'd make this the upstream version. It has indecently faster compile times too.
Indeed, it's a huge improvement! Compilation time on the CI is already halved too. Very impressive.