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

[RUNTIME]crt error handling #5147

Merged
merged 2 commits into from
Mar 27, 2020
Merged

[RUNTIME]crt error handling #5147

merged 2 commits into from
Mar 27, 2020

Conversation

siju-samuel
Copy link
Member

@liangfu @tmoreau89 Please help to review. TIA
Most of the places error was not returned to caller and the function will continue to execute and cause some exception. This is handled.

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

@siju-samuel siju-samuel changed the title [CRT]crt error handling [RUNTIME]crt error handling Mar 25, 2020
Copy link
Member

@liangfu liangfu left a comment

Choose a reason for hiding this comment

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

@siju-samuel Thanks for bringing the improvements. I think we still need to follow MISRA-C rules as closely as possible. Specifically, we should have only one return statement in a function.

reader->BeginArray(reader);
if (!(reader->NextArrayItem(reader))) {
fprintf(stderr, "invalid json format: failed to parse `node_id`\n");
return status;
Copy link
Member

Choose a reason for hiding this comment

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

MISRA-C requires having only one return statement in a function.

@@ -38,25 +38,28 @@ uint32_t Shape_Accumulate(int64_t * shape, uint32_t ndim) {
}

int NodeEntry_Load(TVMGraphRuntimeNodeEntry * entry, JSONReader * reader) {
int status = 0;
int status = -1;
Copy link
Member

Choose a reason for hiding this comment

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

The default is intended to be success, and set status to error when we are in trouble. Subsequently, return status at the end of the function.

Copy link
Member

@liangfu liangfu left a comment

Choose a reason for hiding this comment

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

LGTM

@tmoreau89
Copy link
Contributor

Thanks @siju-samuel for the changes

@tmoreau89 tmoreau89 merged commit 63937a0 into apache:master Mar 27, 2020
@tmoreau89
Copy link
Contributor

Thanks @liangfu @siju-samuel the PR has been merged.

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
* crt error handling

* Review comments fixed
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
* crt error handling

* Review comments fixed
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