win, unix: extract common fields out of the loop fields. #1067
Conversation
include/uv.h
Outdated
| void* check_handles[2]; | ||
| void* async_handles[2]; | ||
| /* Timers */ | ||
| void* idle_handles[2]; |
There was a problem hiding this comment.
move this before prepare, it doesn't belong under the Timers title
There was a problem hiding this comment.
Em, do you want me to udpate the comment too then? Is it (async/ prepare/ check/ idle) watchers?
include/uv.h
Outdated
| uv_async_t wq_async; | ||
| /* The current time according to the event loop. in msecs. */ | ||
| uint64_t time; | ||
| uint64_t timer_counter; |
There was a problem hiding this comment.
put the counter together with the timer stuff
include/uv.h
Outdated
| void* idle_handles[2]; | ||
| /* The current time according to the event loop. in msecs. */ | ||
| uint64_t time; | ||
| uint64_t timer_counter; |
There was a problem hiding this comment.
move this to after the timer heap
|
Small nit, otherwise LGTM. |
|
-30 yeah... squash into one big one? |
txdv
force-pushed
the
diet-loop-struct
branch
from
8462799
to
0a24d7b
Compare
txdv
force-pushed
the
diet-loop-struct
branch
from
0a24d7b
to
fd79f8e
Compare
txdv
force-pushed
the
diet-loop-struct
branch
from
fd79f8e
to
49bd7a5
Compare
include/uv.h
Outdated
| @@ -409,6 +409,37 @@ struct uv_shutdown_s { | |||
| } u; \ | |||
| UV_HANDLE_PRIVATE_FIELDS \ | |||
|
|
|||
| #define UV_TIMER_PRIVATE_FIELDS \ | |||
There was a problem hiding this comment.
come to think of it, we no longer need these private markers if we directly put the fields in the strctures directly. though this way may look a bit nicer. can you check if there are private marked fields that should be extracted?
There was a problem hiding this comment.
can you check if there are private marked fields that should be extracted?
I do not understand this request. Can you elaborate it a bit differently?
There was a problem hiding this comment.
On some handles we have some fields directly in their struct definition which are marked as private with a comment. Check if there are repetitions please.
Also, ignore the first part of my message, I think it looks better with the define, for now.
|
I have found some double definitions in UV_ASYNC_PRIVATE_FIELDS as in, there are fields which are in both unix and windows present, buit not all. UV_FS_PRIVATE_FIELDS has this gem: https://github.com/txdv/libuv/blob/v1.x/include/uv-win.h#L600 Should I pull out the common fields and put them just in the struct definition? Also, what about that todo? It originated from here 514265e |
|
I pulled out common variables of all the other structs. I noticed that sometimes the callbacks in the structs are called just cb, sometimes they are called shutdown_cb(<action_name>_cb). I think <action_name>_cb is better, should I rename all of them in to match this naming style? |
|
Let's leave that for another PR. This one is about the common fields, then we can consolidate naming. |
|
Ok, so the only one left is UV_FS_PRIVATE_FIELDS. That one has a super old TODO on windows and no union on unix (I think it is possible to put most of the fields in unions, depending on what operation is being done, to save some space). I would like @piscisaureus to make a comment on that TODO |
|
No comments, nothing? |
|
Sorry, I have some busy time ahe. I'll come back to this as soon as I can. Thanks for your work! |
|
@saghul can you run the integration tests? |
|
With #1255 I realized that https://github.com/txdv/libuv/blob/slimmer-fs-struct/include/uv.h#L416-L417 timer and repeat should be public fields, should I put them at the top and comment them like that? |
only -11 lines though in total.