higan-emu / libco

libco is a cooperative multithreading library written in C89.
Other
125 stars 25 forks source link

Add way to fixes issue #5 #9 by turn cothread_t into a struct. #14

Closed lygstate closed 4 years ago

lygstate commented 4 years ago

Add way to fixes issue #5 #9 This is a tentative pull request for suggestion, only x86/x64/arm ported yet. When the direction are not wrong, then I will fixes the remaining target.

typedef void (*co_entrypoint)(void);

struct cothread_stct
{
  void* handle;
  void *allocated;
  unsigned int size;
  co_entrypoint entrypoint;
  void *args0;
  int halt;
};
typedef struct cothread_stct cothread_stct;
typedef cothread_stct* cothread_t;
Kawa-oneechan commented 4 years ago

Hard reject from the big man.

That's an extremely hard reject in any case, changing the API. There are many downstream users of libco, some big-name projects. That's actually something where I do want to have some influence, don't break existing users with these changes.

lygstate commented 4 years ago

@Kawa-oneechan gotcha, I won't not change the API, I will create a alternative way

lygstate commented 4 years ago

revert the API back, does co_derive_arg is a good name ?

merryhime commented 4 years ago
  1. Way too many changes in one PR for my taste, this makes it hard to review properly.
  2. co_yield is not necessary: co_switch is intended to be used to "yield" control to a different co_thread
  3. co_yield is a reserved keyword in C++20 and thus must not be used as a name
  4. I do not think changing co_thread to a struct is a good idea. You can implement a halt flag without having to use a struct by checking what the co_thread's rip is. Though in the general case I feel that returning from a co_thread should still be undefined, there isn't always a coroutine to switch to. Remember this library has more use-cases than your narrow use case of uncoordinated coroutines.
  5. Supporting an argument is a fine feature in my personal opinion, but can be implemented without needing to use a struct (just set the correct ABI register during creation).

This library is intended for explicit and coordinated coroutine synchronization. This is intended for applications requiring careful and tight control over coroutine switching. You seem to want to remove the "explicit and controlled" part of this library by making it easier for the uncoordinated use-case. This goes against the spirit of this library and thus any such changes are unlikely to be accepted.

lygstate commented 4 years ago
  1. Way too many changes in one PR for my taste, this makes it hard to review properly.

  2. co_yield is not necessary: co_switch is intended to be used to "yield" control to a different co_thread How about co_switch_main,

  3. co_yield is a reserved keyword in C++20 and thus must not be used as a name

  4. I do not think changing co_thread to a struct is a good idea. You can implement a halt flag without having to use a struct by checking what the co_thread's rip is. Though in the general case I feel that returning from a co_thread should still be undefined, there isn't always a coroutine to switch to. Remember this library has more use-cases than your narrow use case of uncoordinated coroutines. So maybe it's better to place the halt flag and allocated pointer to the top stack? Maybe hidding it from implementation detail?

  5. Supporting an argument is a fine feature in my personal opinion, but can be implemented without needing to use a struct (just set the correct ABI register during creation).

This library is intended for explicit and coordinated coroutine synchronization. This is intended for applications requiring careful and tight control over coroutine switching. You seem to want to remove the "explicit and controlled" part of this library by making it easier for the uncoordinated use-case. This goes against the spirit of this library and thus any such changes are unlikely to be accepted. I didn't remove the "explicit and controlled" part of this library, co_switch are still exist.

merryhime commented 4 years ago

Closed as this is out of scope for this project.

What you wish for can be built on top of libco, it doesn't have to be part of libco itself.

lygstate commented 4 years ago

OK