More templated STL support for the daemon protocol#3895
More templated STL support for the daemon protocol#3895edolstra merged 18 commits intoNixOS:masterfrom
Conversation
This refactor should *not* change the wire protocol.
Template instantiations will cover this case fine.
… into templated-daemon-protocol
…obsidiansystems/nix into templated-daemon-protocol
meditans
force-pushed
the
templated-daemon-protocol
branch
from
d8ebced to
f795f0f
Compare
Ericson2314
changed the title
…stems/nix into templated-daemon-protocol
src/libstore/worker-protocol.hh
Outdated
|
|
||
| void writeOutputPathMap(const Store & store, Sink & out, const OutputPathMap & paths); | ||
| template<typename T> | ||
| struct WorkerProto { |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Changed it back! Should be ready now.
Ericson2314
force-pushed
the
templated-daemon-protocol
branch
from
fabe25f to
c265e0e
Compare
Ericson2314
changed the title
This reverts commit 9ab07e9.
4 participants
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
StorePathCAMapserialization, which I think is OK as #3754 changes all that anyways.