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

include/denc: improvements #12626

Merged
merged 5 commits into from Jan 10, 2017

Conversation

Projects
None yet
3 participants
@adamemerson
Contributor

adamemerson commented Dec 22, 2016

Collapse duplicated code between all containers.

(I know about the test failures, I'll run through those.)

@adamemerson adamemerson requested review from mattbenjamin and liewegas Dec 22, 2016

Foo(buffer::ptr::iterator& p, uint64_t features = 0);
}
Will have be deconde constructble. Instances of this class can be

This comment has been minimized.

@liewegas
efficiently decoded into containers.
Since decode-constructors are new, I require all of them to accept a
features argument, to avoid the combinatorial explosion.

This comment has been minimized.

@liewegas

liewegas Dec 22, 2016

Member

We need to be very careful with this. We generally rely on being able to transition a class from unfeatured to featured and get compile errors on all encoding paths. If callers for this pass in feature fields they aren't explicitly passed (e.g., from a features=0 arg) we may silently fill in bogus features. (Hopefully we haven't already done this elsewhere!)

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 22, 2016

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 22, 2016

The ctor changes are great!

@liewegas liewegas added the common label Dec 27, 2016

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Dec 28, 2016

@liewegas Take another look at this? I plan to redo the constructor stuff on top of it to see if I can get it to work under Trusty Tahr. I don't know if you want to pull this in as is since it's a logically separate change, or hold off until I have the whole thing done.

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 28, 2016

@liewegas liewegas changed the title from DNM: Denc improvements to include/denc: improvements Dec 28, 2016

@liewegas liewegas added the needs-qa label Dec 28, 2016

@liewegas

This comment has been minimized.

Member

liewegas commented Dec 28, 2016

retest this please

1 similar comment
@liewegas

This comment has been minimized.

Member

liewegas commented Dec 28, 2016

retest this please

std::array<legacy_t, 2> s = { legacy_t(1), legacy_t(2) };
test_encode_decode(s);
cout << "std::tuple<uint64_t, uint32_t>" << std::endl;
std::tuple<uint64_t, uint32_t> s = { 100, 97 };

This comment has been minimized.

@tchaikov

tchaikov Dec 29, 2016

Contributor

the jenkins fails. so might want to put something like

std::tuple<uint64_t, uint32_t> s = { 100U, 97U };

instead

}
{
cout << "std::tuple<std::string, uint3_t>" << std::endl;
std::tuple<std::string, uint32_t> s = { "foo", 97 };

This comment has been minimized.

@tchaikov

tchaikov Dec 29, 2016

Contributor

ditto.

}
{
cout << "std::tuple<std::string, std::set<uint32_t>>" << std::endl;
std::tuple<std::string, std::set<uint32_t>> s = { "bar", {1, 2, 3} };

This comment has been minimized.

@tchaikov

tchaikov Dec 29, 2016

Contributor

ditto.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Dec 29, 2016

@liewegas All right! The tests are hammered into submission.

@liewegas

This comment has been minimized.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 4, 2017

@liewegas Do you mean it didn't pass? I see a couple erros but they look like clock skews and problems with the testing infrastructure.

@liewegas

This comment has been minimized.

Member

liewegas commented Jan 9, 2017

I meant it passed; failures do not appear related.

This all looks fine to me. I'm not sure that boost::optional and tuple are that useful, since we aren't explicitly encoding those types, but it's certainly harmless to support them. With optional, for example, it is probably just as likely that we will use a shared flags field to indicate the presence of optional fields. But it does happen...

@liewegas

This comment has been minimized.

Member

liewegas commented Jan 9, 2017

retest this please

adamemerson added some commits Dec 22, 2016

denc: Scrap the container boilerplate!
Using template templates, kill off the reduplication in
container encoding.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
denc: Support boost::container::flat_set
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
denc: Add support for std::array
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
denc: Support boost::optional
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
denc: Support tuples
We could support zero-length tuples, but contiguous appender doesn't
like zero-length writes, so I've ruled them out for now.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

@liewegas liewegas merged commit 9f54e9f into ceph:master Jan 10, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@adamemerson adamemerson deleted the adamemerson:wip-denconstruction branch Jan 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment