Undefined Behavior to access non active union member

[ChuanqiXu9]

Search before asking

• I searched the issues and found no similar issues.

What happened + What you expected to happen

Extract from: https://www.reddit.com/r/cpp/comments/ta4h29/async_simple_a_c20_coroutine_library/:

ContextUnion u;
u.ctx = ctx;

auto prompt =
    _pool.getCurrentId() == (u.id & 0xBFFFFFFF) && opts.prompt;

It is undefined behavior, access to non active union member

Reproduction way

Anything else

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

[qicosmos]

I think we need to check if there are more union UB code.

[qicosmos]

Shell we use std::variant instead of union? however maybe visiting union is more efficient.

[ChuanqiXu9]

I think we need to check if there are more union UB code.

Of course, it is always good to eliminate UB.

Shell we use std::variant instead of union? however maybe visiting union is more efficient.

I think the readability and maintainability is more important given it wouldn't hurt performance much. @RainMark how do you think about?

[RainMark]

the SimpleExecutor's ContextUnion looks like can be removed, static_cast void* to int64_t directly?

And the other tagged union keep it firstly?

[qicosmos]

the SimpleExecutor's ContextUnion looks like can be removed, static_cast void* to int64_t directly?

And the other tagged union keep it firstly?

I think it's acceptable, but we should make sure no UB anymore.

[ChuanqiXu9]

but we should make sure no UB anymore.

This is the right direction. But we couldn't make sure no UB actually : ) Some static analysis tool might help.

[qicosmos]

I agree, union is not safe, we'd better use std::variant as safe union.

[ChuanqiXu9]

Yeah, I agree std::variant is better. But given async_simple is based on practicing. Let's be conservative and refactor it step by step in stead of in one shot.