Skip to content

Conversation

@jimmy-park
Copy link
Contributor

I found more object copies that are not detected by static analyzers after submitting #1767

@yhirose
Copy link
Owner

yhirose commented Feb 8, 2024

@jimmy-park thanks for the further modification. But I have actually been thinking of removing the changes of using std::move unless they are absolutely necessary, since it degrades code readability in my opinion and there are some cases that I don't think we should use std::move.

For instance, I found the following code recently, and it looks dangerous because pop_front() now accesses the moved object.

cpp-httplib/httplib.h

Lines 722 to 723 in 5c00bbf

fn = std::move(pool_.jobs_.front());
pool_.jobs_.pop_front();

Another example is the following.

fn(std::move(key), std::move(val));

cpp-httplib/httplib.h

Lines 3839 to 3842 in 5c00bbf

parse_header(line_reader.ptr(), end,
[&](std::string &&key, std::string &&val) {
headers.emplace(std::move(key), std::move(val));
});

cpp-httplib/httplib.h

Lines 3940 to 3943 in 5c00bbf

parse_header(line_reader.ptr(), end,
[&](std::string &&key, std::string &&val) {
x.headers.emplace(std::move(key), std::move(val));
});

parse_header now passes moved objects to a functor, and a user who supplies a functor needs to know parameters need to be received as &&. I would like to avoid such a hidden dependency that makes the code harder to understand.

Also I simply prefer not seeing std::move too many places in httplib.h. They may give us performance benefit though, I think it's not a big performance gain or it's even unnoticeable. I am now more concerned with the code readability as the httplib.h grows, and would like to keep the code simpler. Also any performance improvements should be measured to see if they are really necessary.

I'll keep this pull request open for now, but I may close it later because of the above reason.
Thanks for your understanding in this matter.

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.

2 participants