Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

capi: Add a note about error code and returned value relation #784

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Apr 12, 2021

Addresses comment #769 (comment)

@@ -250,6 +255,9 @@ typedef struct FizzyImportedGlobal
/// @param error Pointer to store detailed error information at. Can be NULL if error
/// information is not required.
/// @return true if module is valid, false otherwise.
///
/// @note FizzyError::code must be ::FizzySuccess if function returns `true` and must not be
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this not sound like it's a requirement to a user's input?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think so. Would suggest something like "FizzyError::code will be ::FizzySuccess if functions...".

@gumb0 gumb0 requested review from axic and chfast April 12, 2021 12:54
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #784 (d5aac47) into master (c17fb2a) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head d5aac47 differs from pull request most recent head 1211ae6. Consider uploading reports for the commit 1211ae6 to get more accurate results

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   99.24%   99.24%   -0.01%     
==========================================
  Files          79       79              
  Lines       11964    11962       -2     
==========================================
- Hits        11874    11872       -2     
  Misses         90       90              
Flag Coverage Δ
rust 99.87% <ø> (ø)
spectests 90.54% <ø> (ø)
unittests 99.20% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/utils/execute_helpers.hpp 100.00% <0.00%> (ø)

@gumb0 gumb0 force-pushed the error-code-note branch from 1423ffb to 306b04a Compare April 12, 2021 13:40
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Still suggest to use "will be" instead of "must be".

@gumb0 gumb0 force-pushed the error-code-note branch from 306b04a to d5aac47 Compare April 16, 2021 17:01
@@ -260,6 +268,9 @@ bool fizzy_validate(
/// @param error Pointer to store detailed error information at. Can be NULL if error
/// information is not required.
/// @return non-NULL pointer to module in case of success, NULL otherwise.
///
/// @note FizzyError::code will be ::FizzySuccess if function returns non-NULL
/// will and must not be ::FizzySuccess otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the other places used will not be.

@gumb0 gumb0 force-pushed the error-code-note branch from d5aac47 to 256c12b Compare April 19, 2021 09:55
@gumb0 gumb0 force-pushed the error-code-note branch from 256c12b to 1211ae6 Compare April 19, 2021 10:40
@gumb0 gumb0 merged commit 81efc84 into master Apr 19, 2021
@gumb0 gumb0 deleted the error-code-note branch April 19, 2021 12:19
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