[Tests/unit/core] : DataElement improvements #1682
Conversation
msieben
changed the title
|
Question here is do we want to pay for misuse of the set interface? So if the offset is given bigger than the size should we actively set the buffer pointer to a nullptr. A condition we do check with an ASSERT. |
|
That is a verbose explanation without pinpointing certain / the code lines in question. Let me give some motivation. Some ASSERTs have a different purpose, eg, prevent numerical overflow even if the described condition holds, and, the buffer might be wrongly indexed, possibly introducing unintended effects, like for memcpy and memmove. I was told ASSERTS can become NOP by a setting in both Release and Debug mode. Hence the impact can be reduced and still be valuable. In contrast, I have no objections if the code is binned. In that case you are welcome to close the PR. |
For me the ASSERTS are good, defensive code is always good, but I do not prefer to have the if statements tna later on check that what is in the ASSERT again.... |
Source/core/DataElement.h
Outdated
| @@ -167,12 +184,22 @@ namespace Core { | |||
| } | |||
| inline DataElement(const ProxyType<DataStore>& buffer, const uint64_t offset, const uint64_t size = 0) | |||
| : m_Storage(buffer) | |||
| , m_Buffer(&(buffer->Buffer())[offset]) | |||
| , m_Buffer(buffer->Size() > offset ? &(buffer->Buffer())[offset] : nullptr) | |||
There was a problem hiding this comment.
This is for example additional code where later on you will actually (using defensive coding) ASSERT on this. No need to than check it here as well. so This change is additional code for usage outside of the working area of this software AND this is validated later on using an ASSERT. So the ASSERT is great, which makes the additional code here unnecessary.
Source/core/DataElement.h
Outdated
| , m_Offset(offset) | ||
| , m_Size(buffer->Size() - offset) | ||
| , m_Size(buffer->Size() > offset ? buffer->Size() - offset : 0) |
There was a problem hiding this comment.
Same here, validated by an assert. Do not change this.
Source/core/DataElement.h
Outdated
| @@ -206,30 +233,59 @@ namespace Core { | |||
|
|
|||
| inline DataElement(const DataElement& RHS, const uint64_t offset, const uint64_t size = 0) | |||
| : m_Storage(RHS.m_Storage) | |||
| , m_Buffer(&(RHS.m_Buffer[offset])) | |||
| , m_Buffer(RHS.Size() > offset ? &(RHS.m_Buffer[offset]) : nullptr) | |||
There was a problem hiding this comment.
Same here, validated by an assert. Do not change this.
Source/core/DataElement.h
Outdated
| } | ||
| inline DataElement(DataElement&& move, const uint64_t offset, const uint64_t size = 0) | ||
| : m_Storage(std::move(move.m_Storage)) | ||
| , m_Buffer(&(move.m_Buffer[offset])) | ||
| , m_Buffer(move.m_Size > offset ? &(move.m_Buffer[offset]) : nullptr) |
There was a problem hiding this comment.
Same here, validated by an assert. Do not change this.
| // Make sure we are not expanding beyond the size boundary | ||
| ASSERT(size <= (m_MaxSize - m_Size)); | ||
|
|
||
| if ((expanded = Size(m_Size + size)) == true) { |
There was a problem hiding this comment.
This is, I guess a failing testcase. Could you shortly refer to this failing test-case using a JIRA ticket or describe the failing testcase here.
There was a problem hiding this comment.
@pierre, I appreciate the feedback and will make the appropriated changes, including filing the Jira ticket.
No description provided.