UVW v3

[skypjack]

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-commenter]

Codecov Report

Merging #263 (c1e11a8) into master (b2ed37f) will increase coverage by 0.82%. The diff coverage is 97.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.

[stefanofiorentino]

@skypjack do you need any help to get the pipeline back to green too?

[skypjack]

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. 🙂

[sthagen]

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).

[skypjack]

@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! 🙂

[sthagen]

@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:

[skypjack]

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. 😅

[stefanofiorentino]

@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

[skypjack]

Mmm any chance you can reproduce it? I don't know what's going on there otherwise. 😞

[skypjack]

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?

[stefanofiorentino]

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?

[skypjack]

I'm wondering, why we don't have USE_*SAN as github workflows?

Testing a workflow for that on this branch. 👍

[skypjack]

Cool @stefanofiorentino at least I can reproduce the error on the CI. Good catch. 👍

[stefanofiorentino]

@skypjack launching them 1-by-1 with --gtest_filter only Loop.Close causes the leak.

[stefanofiorentino]

Probably the uv_loop_close returns UV_EBUSY. I've to stop investigations for a while.

[skypjack]

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. ❤️

[stefanofiorentino]

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

[skypjack]

What error do you get @stefanofiorentino Loop.walk, a wrong counter?

[stefanofiorentino]

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

[skypjack]

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. 👍

[stefanofiorentino]

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 ;)

[skypjack]

Any other concerns? I'd make this the upstream version. It has indecently faster compile times too.

[stefanofiorentino]

@skypjack we should definitely celebrate this day.

[skypjack]

Indeed, it's a huge improvement! Compilation time on the CI is already halved too. Very impressive.