Skip to content

header_map: copy constructor for HeaderMapImpl. #4129

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

Merged
htuch merged 11 commits into from
Aug 14, 2018
Merged

header_map: copy constructor for HeaderMapImpl. #4129

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
01f7423
header_map: copy constructor for HeaderMapImpl.
htuch Aug 13, 2018
fffdbbc
Mark HeaderList non-copyable, explicit copy assignment operator for T…
htuch Aug 13, 2018
b0aa98a
Kick CI
htuch Aug 13, 2018
e5ce010
Kick CI
htuch Aug 13, 2018
76dfa6c
Add explicit in various places.
htuch Aug 13, 2018
080a830
fix_format
htuch Aug 13, 2018
f88da73
Finally it clicks, we need to prevent implicit move constructor.
htuch Aug 13, 2018
86cc2fa
Better explanation as per @jsedgwick.
htuch Aug 14, 2018
d7d65ad
wip
htuch Aug 14, 2018
3c525eb
More review feedback.
htuch Aug 14, 2018
aa008ff
Comment.
htuch Aug 14, 2018
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
3 changes: 3 additions & 0 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() {
this);
}

HeaderMapImpl::HeaderMapImpl(const HeaderMapImpl& rhs)

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this change still needed? It's unclear to me why the compiler won't just pick the other one.

@htuch htuch Aug 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think so; the implicit copy constructor is deleted, the above constructor for HeaderMap isn't actually a copy constructor.

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. OK.

: HeaderMapImpl(static_cast<const HeaderMap&>(rhs)) {}

@mattklein123 mattklein123 Aug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I'm confused. How is this different from what the compiler did before?

@htuch htuch Aug 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was synthesizing the default copy constructor, since we only supplied a constructor for the class we were deriving from AFAICT.

@htuch htuch Aug 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's something more going on here though, witness the build failures. Looking into this now.


HeaderMapImpl::HeaderMapImpl(
const std::initializer_list<std::pair<LowerCaseString, std::string>>& values)
: HeaderMapImpl() {
Expand Down
1 change: 1 addition & 0 deletions source/common/http/header_map_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class HeaderMapImpl : public HeaderMap {
HeaderMapImpl();
HeaderMapImpl(const std::initializer_list<std::pair<LowerCaseString, std::string>>& values);
HeaderMapImpl(const HeaderMap& rhs);
HeaderMapImpl(const HeaderMapImpl& rhs);

/**
* Add a header via full move. This is the expected high performance paths for codecs populating
Expand Down
17 changes: 17 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,5 +853,22 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) {
}
}

// Validate that [Test]HeaderMapImpl copy construction works. This is a
// regression for where we were missing a valid copy constructor.
TEST(HeaderMapImplTest, HeaderMapImplyCopy) {
{
HeaderMapImpl foo;
foo.addCopy(LowerCaseString("foo"), "bar");
auto headers = std::make_unique<HeaderMapImpl>(foo);
EXPECT_STREQ("bar", headers->get(LowerCaseString("foo"))->value().c_str());
}
{
TestHeaderMapImpl foo;
foo.addCopy("foo", "bar");
auto headers = std::make_unique<TestHeaderMapImpl>(foo);
EXPECT_EQ("bar", headers->get_("foo"));
}
}

} // namespace Http
} // namespace Envoy