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

[NDMatrix] Resolved Dangling Reference Warning #2827

Merged
merged 2 commits into from
Nov 28, 2024
Merged

[NDMatrix] Resolved Dangling Reference Warning #2827

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
569873f
[NDMatrix] Resolved Dangling Reference Warning
AlexandreSinger Nov 27, 2024
a2be7c5
[NDMatrix] Added Comments to NDMatrixProxy
AlexandreSinger Nov 28, 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
22 changes: 14 additions & 8 deletions libs/libvtrutil/src/vtr_ndmatrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ class NdMatrixProxy {
* @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension)
* @param start: Pointer to the start of the sub-matrix this proxy represents
*/
NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_strides, T* start)
AlexandreSinger marked this conversation as resolved.
Show resolved Hide resolved
NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_strides, size_t offset, const std::unique_ptr<T[]>& start)
: dim_sizes_(dim_sizes)
, dim_strides_(dim_strides)
, offset_(offset)
, start_(start) {}

NdMatrixProxy& operator=(const NdMatrixProxy& other) = delete;
Expand All @@ -50,7 +51,8 @@ class NdMatrixProxy {
return NdMatrixProxy<T, N - 1>(
dim_sizes_ + 1, // Pass the dimension information
dim_strides_ + 1, // Pass the stride for the next dimension
start_ + dim_strides_[0] * index); // Advance to index in this dimension
offset_ + dim_strides_[0] * index, // Advance to index in this dimension
start_); // Pass the base pointer.
}

///@brief [] operator
Expand All @@ -62,7 +64,8 @@ class NdMatrixProxy {
private:
const size_t* dim_sizes_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment these data members if you know what they are. Since offset was just added it at least should be commentable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume going from a T* to a unique_ptr reference has some advantage, and if there is that would be a good thing to mention in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment. The main benefit is safety. This makes it clear that the owner of the memory is someone else (the base NDMatrix object). This also allows the base pointer to be reallocated without invalidating all of the NDMatrixProxy objects (since they would be pointing to the stale data). This was the change that helped the compiler better understand what we were doing here. What we were doing was not wrong, just a bit hard to analyze.

const size_t* dim_strides_;
T* start_;
size_t offset_;
const std::unique_ptr<T[]>& start_;
};

///@brief Base case: 1-dimensional array
Expand All @@ -76,9 +79,10 @@ class NdMatrixProxy<T, 1> {
* @param dim_stride: The stride of this dimension (i.e. how many element in memory between indicies of this dimension)
* @param start: Pointer to the start of the sub-matrix this proxy represents
AlexandreSinger marked this conversation as resolved.
Show resolved Hide resolved
*/
NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_stride, T* start)
NdMatrixProxy(const size_t* dim_sizes, const size_t* dim_stride, size_t offset, const std::unique_ptr<T[]>& start)
: dim_sizes_(dim_sizes)
, dim_strides_(dim_stride)
, offset_(offset)
, start_(start) {}

NdMatrixProxy& operator=(const NdMatrixProxy& other) = delete;
Expand All @@ -89,7 +93,7 @@ class NdMatrixProxy<T, 1> {
VTR_ASSERT_SAFE_MSG(index < dim_sizes_[0], "Index out of range (above dimension maximum)");

//Base case
return start_[index];
return start_[offset_ + index];
}

///@brief [] operator
Expand All @@ -108,7 +112,7 @@ class NdMatrixProxy<T, 1> {
* not to clobber elements in other dimensions
*/
const T* data() const {
return start_;
return start_.get() + offset_;
}

///@brief same as above but allow update the value
Expand All @@ -120,7 +124,8 @@ class NdMatrixProxy<T, 1> {
private:
const size_t* dim_sizes_;
AlexandreSinger marked this conversation as resolved.
Show resolved Hide resolved
const size_t* dim_strides_;
T* start_;
size_t offset_;
const std::unique_ptr<T[]>& start_;
};

/**
Expand Down Expand Up @@ -359,7 +364,8 @@ class NdMatrix : public NdMatrixBase<T, N> {
return NdMatrixProxy<T, N - 1>(
this->dim_sizes_.data() + 1, //Pass the dimension information
this->dim_strides_.data() + 1, //Pass the stride for the next dimension
this->data_.get() + this->dim_strides_[0] * index); //Advance to index in this dimension
this->dim_strides_[0] * index, //Advance to index in this dimension
this->data_); //Pass the base pointer
}

/**
Expand Down