Skip to content

[Fix] tokio block#100

Merged
L-jasmine merged 2 commits intoWasmEdge:mainfrom
second-state:fix/tokio_block
Feb 5, 2024
Merged

[Fix] tokio block#100
L-jasmine merged 2 commits intoWasmEdge:mainfrom
second-state:fix/tokio_block

Conversation

@L-jasmine
Copy link
Copy Markdown
Collaborator

No description provided.

Signed-off-by: csh <458761603@qq.com>
Copy link
Copy Markdown
Member

juntao commented Jan 24, 2024

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

The patch introduces several potential issues and lacks sufficient context and documentation. The changes include the modification of types, initialization values, and function implementations without clear justification or explanation. This makes it difficult to understand the purpose of the modifications and evaluate their correctness. Furthermore, the patch lacks proper testing and documentation, leaving doubts about the implementation and potential side effects. It is recommended to address these issues before merging the pull request.

Details

Commit 0d450574830838a18f36283ca452d79711801819

Key changes in the patch:

  • Added imports for AtomicI8 and AtomicU8.
  • Changed the type of SocketWritable from AtomicBool to AtomicI8.
  • Modified the initialization value of SocketWritable to 2 instead of true.
  • Modified the implementation of SocketWritableFuture to accept and check an i8 value instead of a bool.

Potential problems:

  • In the SocketWritable struct, the AtomicI8 type is used but the initialization value is set using the AtomicBool new function. This might cause a type mismatch.
  • The usage of AtomicI8 and AtomicU8 is introduced without any justification or explanation in the patch. It's unclear why these types are needed and whether they are used correctly.
  • The set_writable function in SocketWritable sets the value to 2, which might be an invalid value based on its usage in other parts of the code.
  • The SocketWritableFuture struct now checks if the i8 value is equal to or greater than 0 in the poll function, but it's not clear why this check is necessary and why it's using an i8 type instead of a bool.
  • There are no comments or explanations provided in the patch to clarify the intent or reasoning behind the changes. This makes it difficult to understand the purpose of the modifications and evaluate their correctness.

Overall, the patch introduces several potential issues and lacks sufficient context to properly review and assess the changes.

Commit e45b1e34c058d2fe9def65eee28ff8ff158c22c6

Summary of changes:

  • In the file .github/workflows/ci-build-release-lib.yml, a line of code was modified.

Potential problems:

  • The patch does not provide enough information to fully understand the context and reason for the change.
  • The change is related to installing Wasmedge on Windows, but no explanation is given as to what specifically was fixed.
  • The patch does not include any tests or documentation, so it is unclear if the change was properly tested and documented.
  • The patch includes an email address in the "Signed-off-by" field, which may not be necessary or relevant for a GitHub pull request.

Overall, it would be beneficial to have more context and information about the change, as well as proper testing and documentation to ensure the fix is implemented correctly.

Signed-off-by: csh <458761603@qq.com>
@L-jasmine L-jasmine requested a review from apepkuss January 24, 2024 06:34
@L-jasmine L-jasmine merged commit 1ef2434 into WasmEdge:main Feb 5, 2024
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