Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor pc_tranc.c #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor pc_tranc.c #62

wants to merge 1 commit into from

Conversation

xpol
Copy link
Contributor

@xpol xpol commented Mar 10, 2016

  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
Copy link
Contributor Author

xpol commented Mar 18, 2016

@cynron Any feedbacks?

@cynron
Copy link
Member

cynron commented Mar 19, 2016

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

@cynron
Copy link
Member

cynron commented Apr 25, 2016

@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
Copy link
Contributor Author

xpol commented Apr 25, 2016

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
Copy link
Contributor Author

xpol commented Apr 29, 2016

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
Copy link
Member

cynron commented Apr 29, 2016

#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