Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/manual/src/SUMMARY.md.in
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
28 changes: 28 additions & 0 deletions doc/manual/src/contributing/cxx.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion src/build-remote/build-remote.cc
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ static int main_build_remote(int argc, char * * argv)
!trusted || *trusted;
});

// See the very large comment in `case wopBuildDerivation:` in
// See the very large comment in `case WorkerProto::Op::BuildDerivation:` in
// `src/libstore/daemon.cc` that explains the trust model here.
//
// This condition mirrors that: that code enforces the "rules" outlined there;
Expand Down
7 changes: 5 additions & 2 deletions src/libstore/build/derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1150,9 +1151,11 @@ HookReply DerivationGoal::tryBuildHook()
throw;
}

WorkerProto::WriteConn conn { hook->sink };

/* Tell the hook all the inputs that have to be copied to the
remote system. */
workerProtoWrite(worker.store, hook->sink, inputPaths);
WorkerProto::write(worker.store, conn, inputPaths);

/* Tell the hooks the missing outputs that have to be copied back
from the remote system. */
Expand All @@ -1163,7 +1166,7 @@ HookReply DerivationGoal::tryBuildHook()
if (buildMode != bmCheck && status.known && status.known->isValid()) continue;
missingOutputs.insert(outputName);
}
workerProtoWrite(worker.store, hook->sink, missingOutputs);
WorkerProto::write(worker.store, conn, missingOutputs);
}

hook->sink = FdSink();
Expand Down
1 change: 0 additions & 1 deletion src/libstore/build/local-derivation-goal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "archive.hh"
#include "compression.hh"
#include "daemon.hh"
#include "worker-protocol.hh"
#include "topo-sort.hh"
#include "callback.hh"
#include "json-utils.hh"
Expand Down
Loading