libpomelo2 icon indicating copy to clipboard operation
libpomelo2 copied to clipboard

Refactor pc_tranc.c

Open xpol opened this issue 9 years ago • 6 comments

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

xpol avatar Mar 10 '16 13:03 xpol

@cynron Any feedbacks?

xpol avatar Mar 18 '16 08:03 xpol

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

cynron avatar Mar 19 '16 00:03 cynron

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

cynron avatar Apr 25 '16 08:04 cynron

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 avatar Apr 25 '16 09:04 xpol

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

xpol avatar Apr 29 '16 02:04 xpol

#66 is merged, awesome

cynron avatar Apr 29 '16 08:04 cynron