Skip to content

[SYCL][Graph] Correct assert usage #10969

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.

Sign up for GitHub

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

Merged
dm-vodopyanov merged 3 commits into from
Aug 28, 2023
Merged

[SYCL][Graph] Correct assert usage #10969

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0254bda
[SYCL][Graphs] Correct assert usage
Aug 25, 2023
31df6da
[SYCL][Graph] Replacing assert by use of at function
Aug 26, 2023
65509a9
Merge remote-tracking branch 'origin/sycl' into assert_fix
Aug 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sycl/source/detail/graph_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ std::shared_ptr<node_impl> graph_impl::addSubgraphNodes(
auto Successor = NodesMap[NextNode];
NodeCopy->registerSuccessor(Successor, NodeCopy);
} else {
assert("Node duplication failed. A duplicated node is missing.");
assert(false &&
"Node duplication failed. A duplicated node is missing.");
}

@bader bader Aug 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to if statement in this case. Just move the condition to assert.

assert(NodesMap.find(NextNode) != NodesMap.end() && "Node duplication failed. A duplicated node is missing.");
auto Successor = NodesMap[NextNode];
NodeCopy->registerSuccessor(Successor, NodeCopy);

@aelovikov-intel aelovikov-intel Aug 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't aware of this fix because it wasn't linked from the guilty PR, so I've created #10977 that does exactly what @bader asks for.

@bader bader Aug 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can use at instead of operator [] to make out-of-bounds check by std::map and remove assert from the code? NOTE: this will enable the check in Release mode too, which might be not intended.

@EwanC EwanC Aug 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@reble has pushed an update so that this PR uses at

}
}
Expand Down