Split out worker protocol template definitions from declarations
This is generally a fine practice: Putting implementations in headers makes them harder to read and slows compilation. Unfortunately it is necessary for templates, but we can ameliorate that by putting them in a separate header. Only files which need to instantiate those templates will need to include the header with the implementation; the rest can just include the declaration. This is now documenting in the contributing guide. Also, it just happens that these polymorphic serializers are the protocol agnostic ones. (Worker and serve protocol have the same logic for these container types.) This means by doing this general template cleanup, we are also getting a head start on better indicating which code is protocol-specific and which code is shared between protocols.
This commit is contained in:
parent
f5e620bf2b
commit
469d06f9bc
15 changed files with 120 additions and 63 deletions
|
@ -106,6 +106,7 @@
|
|||
- [Hacking](contributing/hacking.md)
|
||||
- [Experimental Features](contributing/experimental-features.md)
|
||||
- [CLI guideline](contributing/cli-guideline.md)
|
||||
- [C++ style guide](contributing/cxx.md)
|
||||
- [Release Notes](release-notes/release-notes.md)
|
||||
- [Release X.Y (202?-??-??)](release-notes/rl-next.md)
|
||||
- [Release 2.16 (2023-05-31)](release-notes/rl-2.16.md)
|
||||
|
|
28
doc/manual/src/contributing/cxx.md
Normal file
28
doc/manual/src/contributing/cxx.md
Normal file
|
@ -0,0 +1,28 @@
|
|||
# C++ style guide
|
||||
|
||||
Some miscellaneous notes on how we write C++.
|
||||
Formatting we hope to eventually normalize automatically, so this section is free to just discuss higher-level concerns.
|
||||
|
||||
## The `*-impl.hh` pattern
|
||||
|
||||
Let's start with some background info first.
|
||||
Headers, are supposed to contain declarations, not definitions.
|
||||
This allows us to change a definition without changing the declaration, and have a very small rebuild during development.
|
||||
Templates, however, need to be specialized to use-sites.
|
||||
Absent fancier techniques, templates require that the definition, not just mere declaration, must be available at use-sites in order to make that specialization on the fly as part of compiling those use-sites.
|
||||
Making definitions available like that means putting them in headers, but that is unfortunately means we get all the extra rebuilds we want to avoid by just putting declarations there as described above.
|
||||
|
||||
The `*-impl.hh` pattern is a ham-fisted partial solution to this problem.
|
||||
It constitutes:
|
||||
|
||||
- Declaring items only in the main `foo.hh`, including templates.
|
||||
|
||||
- Putting template definitions in a companion `foo-impl.hh` header.
|
||||
|
||||
Most C++ developers would accompany this by having `foo.hh` include `foo-impl.hh`, to ensure any file getting the template declarations also got the template definitions.
|
||||
But we've found not doing this has some benefits and fewer than imagined downsides.
|
||||
The fact remains that headers are rarely as minimal as they could be;
|
||||
there is often code that needs declarations from the headers but not the templates within them.
|
||||
With our pattern where `foo.hh` doesn't include `foo-impl.hh`, that means they can just include `foo.hh`
|
||||
Code that needs both just includes `foo.hh` and `foo-impl.hh`.
|
||||
This does make linking error possible where something forgets to include `foo-impl.hh` that needs it, but those are build-time only as easy to fix.
|
|
@ -9,6 +9,7 @@
|
|||
#include "archive.hh"
|
||||
#include "compression.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "topo-sort.hh"
|
||||
#include "callback.hh"
|
||||
#include "local-store.hh" // TODO remove, along with remaining downcasts
|
||||
|
|
|
@ -11,6 +11,7 @@
|
|||
#include "compression.hh"
|
||||
#include "daemon.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "topo-sort.hh"
|
||||
#include "callback.hh"
|
||||
#include "json-utils.hh"
|
||||
|
|
|
@ -1,6 +1,7 @@
|
|||
#include "daemon.hh"
|
||||
#include "monitor-fd.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "build-result.hh"
|
||||
#include "store-api.hh"
|
||||
#include "store-cast.hh"
|
||||
|
|
|
@ -5,6 +5,7 @@
|
|||
#include "util.hh"
|
||||
#include "split.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "fs-accessor.hh"
|
||||
#include <boost/container/small_vector.hpp>
|
||||
#include <nlohmann/json.hpp>
|
||||
|
|
|
@ -2,6 +2,7 @@
|
|||
#include "store-api.hh"
|
||||
#include "archive.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
|
||||
#include <algorithm>
|
||||
|
||||
|
|
|
@ -7,6 +7,7 @@
|
|||
#include "store-api.hh"
|
||||
#include "path-with-outputs.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "ssh.hh"
|
||||
#include "derivations.hh"
|
||||
#include "callback.hh"
|
||||
|
|
|
@ -1,5 +1,6 @@
|
|||
#include "path-info.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "store-api.hh"
|
||||
|
||||
namespace nix {
|
||||
|
|
|
@ -6,6 +6,7 @@
|
|||
#include "build-result.hh"
|
||||
#include "remote-store.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "archive.hh"
|
||||
#include "globals.hh"
|
||||
#include "derivations.hh"
|
||||
|
|
78
src/libstore/worker-protocol-impl.hh
Normal file
78
src/libstore/worker-protocol-impl.hh
Normal file
|
@ -0,0 +1,78 @@
|
|||
#pragma once
|
||||
/**
|
||||
* @file
|
||||
*
|
||||
* Template implementations (as opposed to mere declarations).
|
||||
*
|
||||
* This file is an exmample of the "impl.hh" pattern. See the
|
||||
* contributing guide.
|
||||
*/
|
||||
|
||||
#include "worker-protocol.hh"
|
||||
|
||||
namespace nix {
|
||||
|
||||
template<typename T>
|
||||
std::vector<T> WorkerProto<std::vector<T>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::vector<T> resSet;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
resSet.push_back(WorkerProto<T>::read(store, from));
|
||||
}
|
||||
return resSet;
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
void WorkerProto<std::vector<T>>::write(const Store & store, Sink & out, const std::vector<T> & resSet)
|
||||
{
|
||||
out << resSet.size();
|
||||
for (auto & key : resSet) {
|
||||
WorkerProto<T>::write(store, out, key);
|
||||
}
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
std::set<T> WorkerProto<std::set<T>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::set<T> resSet;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
resSet.insert(WorkerProto<T>::read(store, from));
|
||||
}
|
||||
return resSet;
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
void WorkerProto<std::set<T>>::write(const Store & store, Sink & out, const std::set<T> & resSet)
|
||||
{
|
||||
out << resSet.size();
|
||||
for (auto & key : resSet) {
|
||||
WorkerProto<T>::write(store, out, key);
|
||||
}
|
||||
}
|
||||
|
||||
template<typename K, typename V>
|
||||
std::map<K, V> WorkerProto<std::map<K, V>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::map<K, V> resMap;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
auto k = WorkerProto<K>::read(store, from);
|
||||
auto v = WorkerProto<V>::read(store, from);
|
||||
resMap.insert_or_assign(std::move(k), std::move(v));
|
||||
}
|
||||
return resMap;
|
||||
}
|
||||
|
||||
template<typename K, typename V>
|
||||
void WorkerProto<std::map<K, V>>::write(const Store & store, Sink & out, const std::map<K, V> & resMap)
|
||||
{
|
||||
out << resMap.size();
|
||||
for (auto & i : resMap) {
|
||||
WorkerProto<K>::write(store, out, i.first);
|
||||
WorkerProto<V>::write(store, out, i.second);
|
||||
}
|
||||
}
|
||||
|
||||
}
|
|
@ -4,6 +4,7 @@
|
|||
#include "store-api.hh"
|
||||
#include "build-result.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "archive.hh"
|
||||
#include "derivations.hh"
|
||||
|
||||
|
|
|
@ -173,67 +173,4 @@ MAKE_WORKER_PROTO(std::optional<StorePath>);
|
|||
template<>
|
||||
MAKE_WORKER_PROTO(std::optional<ContentAddress>);
|
||||
|
||||
template<typename T>
|
||||
std::vector<T> WorkerProto<std::vector<T>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::vector<T> resSet;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
resSet.push_back(WorkerProto<T>::read(store, from));
|
||||
}
|
||||
return resSet;
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
void WorkerProto<std::vector<T>>::write(const Store & store, Sink & out, const std::vector<T> & resSet)
|
||||
{
|
||||
out << resSet.size();
|
||||
for (auto & key : resSet) {
|
||||
WorkerProto<T>::write(store, out, key);
|
||||
}
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
std::set<T> WorkerProto<std::set<T>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::set<T> resSet;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
resSet.insert(WorkerProto<T>::read(store, from));
|
||||
}
|
||||
return resSet;
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
void WorkerProto<std::set<T>>::write(const Store & store, Sink & out, const std::set<T> & resSet)
|
||||
{
|
||||
out << resSet.size();
|
||||
for (auto & key : resSet) {
|
||||
WorkerProto<T>::write(store, out, key);
|
||||
}
|
||||
}
|
||||
|
||||
template<typename K, typename V>
|
||||
std::map<K, V> WorkerProto<std::map<K, V>>::read(const Store & store, Source & from)
|
||||
{
|
||||
std::map<K, V> resMap;
|
||||
auto size = readNum<size_t>(from);
|
||||
while (size--) {
|
||||
auto k = WorkerProto<K>::read(store, from);
|
||||
auto v = WorkerProto<V>::read(store, from);
|
||||
resMap.insert_or_assign(std::move(k), std::move(v));
|
||||
}
|
||||
return resMap;
|
||||
}
|
||||
|
||||
template<typename K, typename V>
|
||||
void WorkerProto<std::map<K, V>>::write(const Store & store, Sink & out, const std::map<K, V> & resMap)
|
||||
{
|
||||
out << resMap.size();
|
||||
for (auto & i : resMap) {
|
||||
WorkerProto<K>::write(store, out, i.first);
|
||||
WorkerProto<V>::write(store, out, i.second);
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -4,6 +4,9 @@
|
|||
*
|
||||
* Template implementations (as opposed to mere declarations).
|
||||
*
|
||||
* This file is an exmample of the "impl.hh" pattern. See the
|
||||
* contributing guide.
|
||||
*
|
||||
* One only needs to include this when one is declaring a
|
||||
* `BaseClass<CustomType>` setting, or as derived class of such an
|
||||
* instantiation.
|
||||
|
|
|
@ -12,6 +12,7 @@
|
|||
#include "shared.hh"
|
||||
#include "util.hh"
|
||||
#include "worker-protocol.hh"
|
||||
#include "worker-protocol-impl.hh"
|
||||
#include "graphml.hh"
|
||||
#include "legacy.hh"
|
||||
#include "path-with-outputs.hh"
|
||||
|
|
Loading…
Reference in a new issue