Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Test for set_final_data(..) method #122

Merged
merged 5 commits into from
Feb 3, 2021

Conversation

maximdimakov
Copy link

Signed-off-by: mdimakov [email protected]

Copy link

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Looks good.
Though, should other types be tested here as well to ensure that there is no failure?

if (data_vector[i] != true)
assert(false);
}
}

Choose a reason for hiding this comment

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

Lacks return 0;

@s-kanaev
Copy link

Also, there're tests for set_final_data in https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Basic/buffer/buffer.cpp.
I believe, this test can be added there.


for (size_t i = 0; i < size; i++) {
if (data_vector[i] != true)
assert(false);

Choose a reason for hiding this comment

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

can you write some message and return 1 to be more user friendly?

@maximdimakov
Copy link
Author

@s-kanaev The test was moved to the buffer.cpp. It was extracted for int and double types.
@vladimirlaz Assert message was added

romanovvlad pushed a commit to intel/llvm that referenced this pull request Feb 1, 2021
Fixed set_final_data() taking std::vector<bool> which was not compiling because
such a vector has no data method. The patch adds a special case for bool type.

Test: intel/llvm-test-suite#122

Signed-off-by: mdimakov [email protected]
@s-kanaev s-kanaev requested a review from vladimirlaz February 2, 2021 20:09
@vladimirlaz vladimirlaz merged commit 12688e3 into intel:intel Feb 3, 2021
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants