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.

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

http_utils update using std::span #3300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

devnexen
Copy link
Collaborator

No description provided.

@nunojpg
Copy link
Collaborator

nunojpg commented Feb 18, 2023

We are storing a copy of the data at m_body, not a reference/pointer.

With std::span you just store a pointer, to you need to make sure the original passed data remains available.

Think of it as string vs string_view.

@@ -43,7 +43,7 @@ X509_CRL::X509_CRL(DataSource& src)
load_data(src);
}

X509_CRL::X509_CRL(const std::vector<uint8_t>& vec)
X509_CRL::X509_CRL(std::span<const uint8_t> vec)
{
DataSource_Memory src(vec.data(), vec.size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DataSource_Memory src(vec.data(), vec.size());
DataSource_Memory src(vec);

DataSource_Memory already learned to deal with std::span<>

@lieser
Copy link
Collaborator

lieser commented Feb 20, 2023

We are storing a copy of the data at m_body, not a reference/pointer.

I agree that we should be careful about this. And make really sure that not storing a copy is what we want and brings us enough performance that we care about that it is worth the potential lifetime issue.

If we stay with the current storing a copy of data in Response, we should change the constructor to takes a std::vector<uint8_t> (and not a reference). If the caller no longer needs the body this would allow to avoid creating a copy of the body without the lifetime issue just storing a pointer to it has.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants