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.

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

Add get_upstream_resource method to stream_checking_resource_adaptor #15203

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Add get_upstream_resource method to stream_checking_resource_adaptor #15203

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3b868b1
Add `get_upstream_resource` method to `stream_checking_resource_adaptor`
miscco Mar 1, 2024
84a153f
Keep documentation
miscco Mar 1, 2024
77236c8
Do not use briefreturn for comments
miscco Mar 1, 2024
a38d734
Typos...
miscco Mar 1, 2024
1df1d0a
Update cpp/include/cudf_test/stream_checking_resource_adaptor.hpp
miscco Mar 3, 2024
ab2cbb6
For now do not deprecated `get_upstream`
miscco Mar 5, 2024
50755f3
Merge branch 'branch-24.04' into stream_checking_resource_adaptor
miscco Mar 5, 2024
95588c6
delete unused method
miscco Mar 6, 2024
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
23 changes: 17 additions & 6 deletions cpp/include/cudf_test/stream_checking_resource_adaptor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <cudf/detail/utilities/stacktrace.hpp>

#include <rmm/mr/device/device_memory_resource.hpp>
#include <rmm/resource_ref.hpp>

#include <iostream>

Expand Down Expand Up @@ -58,11 +59,21 @@ class stream_checking_resource_adaptor final : public rmm::mr::device_memory_res
default;

/**
* @brief Return pointer to the upstream resource.
*
* @return Pointer to the upstream resource.
* @briefreturn{rmm::device_async_resource_ref to the upstream resource}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can yeah, I would note that this is what we settled on in rmm

Copy link
Member

Choose a reason for hiding this comment

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

This macro was created by @vyasr to clean up repetitive documentation in exactly this situation of documenting getters. Why did you want it changed back, @davidwendt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a standard doxygen tag. This macro is implemented in rmm but not cudf.
This header is not included in the published docs (and not actually processed by doxygen) so I'd rather it be more decipherable to humans in the source text here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought the macro was centralized for RAPIDS. I'm guessing @vyasr will add it once cuDF moves to unified C++/Python docs.

*/
[[nodiscard]] rmm::device_async_resource_ref get_upstream_resource() const noexcept
{
return upstream_;
}

/**
* @briefreturn{Upstream* to the upstream memory resource}
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too.

*/
Upstream* get_upstream() const noexcept { return upstream_; }
[[deprecated("Use get_upstream_resource instead")]] [[nodiscard]] Upstream* get_upstream()
const noexcept
{
return upstream_;
}

private:
/**
Expand Down Expand Up @@ -110,8 +121,8 @@ class stream_checking_resource_adaptor final : public rmm::mr::device_memory_res
{
if (this == &other) { return true; }
auto cast = dynamic_cast<stream_checking_resource_adaptor<Upstream> const*>(&other);
return cast != nullptr ? upstream_->is_equal(*cast->get_upstream())
: upstream_->is_equal(other);
if (cast == nullptr) { return upstream_->is_equal(other); }
return get_upstream_resource() == cast->get_upstream_resource();
}

/**
Expand Down
Loading