Skip to content

Conversation

@yamt
Copy link
Collaborator

@yamt yamt commented Aug 5, 2022

ASSERT_NOT_IMPLEMENTED is bh_assert, which might be no-op.
in that case, it's better to fall back to the "default" case,
which reports an error properly.

}
case IMPORT_KIND_MEMORY:
case IMPORT_KIND_TABLE:
ASSERT_NOT_IMPLEMENTED();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leads to compilation warning in debug mode: warning: this statement may fall through
How about changing to:

            case IMPORT_KIND_MEMORY:
            case IMPORT_KIND_TABLE:
                ASSERT_NOT_IMPLEMENTED();
                goto handle_default;
            handle_default:
            default:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's unfortunate that clang doesn't recognize the traditional /* FALLTHROUGH */.
how do you think about introducing something like #defile BH_FALLTHROUGH __attribute__((fallthrough))?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had better not, it might be not friendly to some compilers.
We have the similar case in:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_emit_numberic.c#L455-L458
The modification such as that should be OK.

Or how about handling default together with case IMPORT_KIND_MEMORY/IMPORT_KIND_TABLE:

            case IMPORT_KIND_MEMORY:
            case IMPORT_KIND_TABLE:
            default:
                  ASSERT_NOT_IMPLEMENTED();
                  LOG_WARNING("%s meets unsupported kind: %d", __FUNCTION__,
                              import_rt->kind);
                  goto failed;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had better not, it might be not friendly to some compilers. We have the similar case in: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_emit_numberic.c#L455-L458 The modification such as that should be OK.

ok

Or how about handling default together with case IMPORT_KIND_MEMORY/IMPORT_KIND_TABLE:

            case IMPORT_KIND_MEMORY:
            case IMPORT_KIND_TABLE:
            default:
                  ASSERT_NOT_IMPLEMENTED();
                  LOG_WARNING("%s meets unsupported kind: %d", __FUNCTION__,
                              import_rt->kind);
                  goto failed;

i think it makes the most sense this particular case. done.

break;
case WASM_EXTERN_MEMORY:
case WASM_EXTERN_TABLE:
ASSERT_NOT_IMPLEMENTED();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

ASSERT_NOT_IMPLEMENTED is bh_assert, which might be no-op.
in that case, it's better to fall back to the "default" case,
which reports an error properly.
@yamt yamt force-pushed the wasm-c-api-not-implemented branch from 5c065f3 to 60bd4b5 Compare August 8, 2022 06:31
@wenyongh wenyongh merged commit 425efb8 into bytecodealliance:main Aug 8, 2022
loganek pushed a commit to loganek/wasm-micro-runtime that referenced this pull request Aug 31, 2022
ASSERT_NOT_IMPLEMENTED is bh_assert, which might be no-op.
in that case, it's better to fall back to the "default" case,
which reports an error properly.
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
ASSERT_NOT_IMPLEMENTED is bh_assert, which might be no-op.
in that case, it's better to fall back to the "default" case,
which reports an error properly.
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.

3 participants