Skip to content

Refactor pc_tranc.c#62

Open
xpol wants to merge 1 commit into
NetEase:masterfrom
xpol:refactor-pc-trans
Open

Refactor pc_tranc.c#62
xpol wants to merge 1 commit into
NetEase:masterfrom
xpol:refactor-pc-trans

Conversation

@xpol

@xpol xpol commented Mar 10, 2016

Copy link
Copy Markdown
Contributor
  1. split sync and async code in pc_tranc.c into different functions.
  2. pc__trans_resp, pc__trans_sent and pc__trans_fire_event will not queue items, the pending arg has removed.
  3. do state change in pc_trans_fire_event, that is when event occurs, the state is changed not when the event is dispatched. ( this shuld fixes Assertion fail when connect and disconnect then cleanup without delay. #60 )
  4. using a simple state machine for state translate code in pc_trans_fire_event.

TODO: remove extra indent for code marked with // follow code has extra indent for better git diff in pull request. after the pr is merged.

1. split sync and async code into different functions.
2. `pc__trans_resp`, `pc__trans_sent` and `pc__trans_fire_event` will not queue items, the pending arg has removed.
3. do state change in  `pc_trans_fire_event` that is when event occurs, the state is changed not when the event his dispatched.
4. using a simple state machine for state change code in `pc_trans_fire_event`.

TODO:   remove extra indent for code marked with  `// follow code has extra indent for better git diff in pull request.` after the pr is merged.
@xpol

xpol commented Mar 18, 2016

Copy link
Copy Markdown
Contributor Author

@cynron Any feedbacks?

@cynron

cynron commented Mar 19, 2016

Copy link
Copy Markdown
Member

sorry, I am too busy these days, I will review this heavy PR carefully later.

@cynron

cynron commented Apr 25, 2016

Copy link
Copy Markdown
Member

@xpol

1. split sync and async code in pc_tranc.c into different functions.
2. pc__trans_resp, pc__trans_sent and pc__trans_fire_event will not queue items, the pending arg has removed.

I think this change is not neccessary as the orignal works well, but even so, I think it should be in a seperated commit

3. do state change in pc_trans_fire_event, that is when event occurs, the state is changed not when the event is dispatched. ( this shuld fixes #60 )

awesome, as we had not used poll mode heavily, the poll mode is not well-tested in our use case. it is a bug, good catch

4. using a simple state machine for state translate code in pc_trans_fire_event.

this looks good, but I think the impl can be better:

  • macro RETURN_IF using VAR_ARGS which may not work on some old compilers ?
  • more comment on the state trans table ?
  • state_trans_table_entry may be a better name than state_tans_s ?
  • represent state_trans_s.allowd in bits not an array? I think short is long enough
  • some syntax error in error message ?

Thank you!

@xpol

xpol commented Apr 25, 2016

Copy link
Copy Markdown
Contributor Author

represent state_trans_s.allowd in bits not an array? I think short is long enough

I think its just use some bytes, but provide more readability and simple code.

@xpol

xpol commented Apr 29, 2016

Copy link
Copy Markdown
Contributor Author

I think this change is not neccessary as the orignal works well, but even so, I think it should be in a seperated commit

IMHO, works well is not sufficient reason for doing sync and async in one single function.
And I agree, that it should go to a seprated PR ( #66 ).

@cynron

cynron commented Apr 29, 2016

Copy link
Copy Markdown
Member

#66 is merged, awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion fail when connect and disconnect then cleanup without delay.

2 participants