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 all commits
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
53 changes: 42 additions & 11 deletions libs/libvtrutil/src/vtr_ndmatrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,14 @@ class NdMatrixProxy {
* @brief Construct a matrix proxy object
*
* @param dim_sizes: Array of dimension sizes
* @param idim: The dimension associated with this proxy
* @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
* @param offset: The offset from the start that this sub-matrix starts at.
* @param start: Pointer to the start of the base NDMatrix of this proxy
*/
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 @@ -60,9 +62,22 @@ class NdMatrixProxy {
}

private:
/// @brief The sizes of each dimension of this proxy. This is an array of
/// length N.
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.


/// @brief The stride of each dimension of this proxy. This is an array of
/// length N.
const size_t* dim_strides_;
T* start_;

/// @brief The offset from the base NDMatrix object that this sub-matrix
/// starts at.
size_t offset_;

/// @brief The pointer to the start of the base NDMatrix data. Since the
/// base NDMatrix object owns the memory, we hold onto a reference
/// to its unique pointer. This is safer than passing a bare pointer.
const std::unique_ptr<T[]>& start_;
};

///@brief Base case: 1-dimensional array
Expand All @@ -74,11 +89,13 @@ class NdMatrixProxy<T, 1> {
*
* @param dim_sizes: Array of dimension sizes
* @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
* @param offset: The offset from the start that this sub-matrix starts at.
* @param start: Pointer to the start of the base NDMatrix of this proxy
*/
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 +106,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 +125,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 @@ -118,9 +135,22 @@ class NdMatrixProxy<T, 1> {
}

private:
/// @brief The sizes of each dimension of this proxy. This is an array of
/// length N.
const size_t* dim_sizes_;
AlexandreSinger marked this conversation as resolved.
Show resolved Hide resolved

/// @brief The stride of each dimension of this proxy. This is an array of
/// length N.
const size_t* dim_strides_;
T* start_;

/// @brief The offset from the base NDMatrix object that this sub-matrix
/// starts at.
size_t offset_;

/// @brief The pointer to the start of the base NDMatrix data. Since the
/// base NDMatrix object owns the memory, we hold onto a reference
/// to its unique pointer. This is safer than passing a bare pointer.
const std::unique_ptr<T[]>& start_;
};

/**
Expand Down Expand Up @@ -359,7 +389,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