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
More templated STL support for the daemon protocol #3895
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
d8ebced
to
f795f0f
Compare
…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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it back! Should be ready now.
fabe25f
to
c265e0e
Compare
This reverts commit 9ab07e9.
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.