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

More templated STL support for the daemon protocol #3895

Merged
merged 18 commits into from Oct 5, 2020

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 4, 2020

It's a little boilerplate up front, but I think it makes things much easier, later.

I'm currently getting an invalid tag for a std::optional, which is stumping me as it's on a code path that I thought was operationally identical to the old one. Even weirder that the failure is Linux-specific.

This only intentionally changes the protocol for the StorePathCAMap serialization, which I think is OK as #3754 changes all that anyways.

@meditans meditans force-pushed the templated-daemon-protocol branch from d8ebced to f795f0f Aug 6, 2020
@Ericson2314 Ericson2314 changed the title WIP: More templated STL support for the daemon protocol --- contains #3859 More templated STL support for the daemon protocol --- contains #3859 Aug 7, 2020
src/libstore/worker-protocol.hh Outdated Show resolved Hide resolved

void writeOutputPathMap(const Store & store, Sink & out, const OutputPathMap & paths);
template<typename T>
struct WorkerProto {
Copy link
Contributor

@regnat regnat Aug 10, 2020

What's the motivation for using a templated struct here rather than a namespace like it was done in #3859 (with templated functions inside). Nothing fundamental, but I feel like worker_proto::read<StorePath>() is more natural than WorkerProto<StorePath>::read()

Copy link
Member Author

@Ericson2314 Ericson2314 Aug 10, 2020

We were worried about implicit conversation so we wanted to "recur" with an explicit <T> template instantiation.

However implicit conversations ended up not being a problem, and I've made it so the most important PRs no longer depend on this one so we can go either way.

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 18, 2020

@edolstra What do you think about switching from the workspace to WorkerProto? Happy to revert that part if you don't like it. As I wrote above I introduced it because i thought there was a bug. I'm 50-50 on whether to keep it or not.

Copy link
Member

@edolstra edolstra Sep 28, 2020

I think I would prefer keeping worker_proto. Seeing WorkerProto objects constructed everywhere is kind of confusing, since it requires you to read the WorkerProto definition to see that they're just a zero-cost wrapper around static methods...

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 28, 2020

Changing it back, but for the record I believe Class::method, which is what I'm doing here, syntax never constructs an object of that class?

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 30, 2020

Changed it back! Should be ready now.

@Ericson2314 Ericson2314 force-pushed the templated-daemon-protocol branch from fabe25f to c265e0e Aug 20, 2020
@Ericson2314 Ericson2314 changed the title More templated STL support for the daemon protocol --- contains #3859 More templated STL support for the daemon protocol Aug 20, 2020
@edolstra edolstra merged commit f3aba88 into NixOS:master Oct 5, 2020
2 checks passed
@Ericson2314 Ericson2314 deleted the templated-daemon-protocol branch Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants