Refactor InstallPaths API and comments a little. #4341
Conversation
015d0a1 to
a2629f5
Compare
| // | ||
| // When run from bazel (for example, in unit tests or development binaries) | ||
| // this will look like: | ||
| // `bazel-bin/some/bazel/target.runfiles/_main/toolchain/install/prefix_root` |
There was a problem hiding this comment.
The prefix_root at the end looks a little suspicious to me. Is that right?
There was a problem hiding this comment.
We use an explicit prefix_root for all the things installed in the package to keep them distinct from the code and other things in the install package for managing an installation.
| @@ -16,6 +16,14 @@ | |||
| #include "tools/cpp/runfiles/runfiles.h" | |||
|
|
|||
| namespace Carbon { | |||
|
|
|||
| class InstallPathsTestPeer { | |||
There was a problem hiding this comment.
(not for this PR, maybe for a follow-up)
Is "test peer" a well established term? I couldn't find it when searching, and it was new to me. And the term "peer" is specific enough that I somewhat expect it to have a fairly specific meaning.
If its not that established, I'm still perfectly happy with the pattern itself to allow testing -- but maybe ...TestHelper as a name to be a little more generic? But as I said, not this PR, a follow-up to maybe rename both if folks like that.
There was a problem hiding this comment.
Peer specifically comes from https://abseil.io/tips/135
There was a problem hiding this comment.
Note, left to my own devices, I'd just make InstallPathsTest itself the friend... (I would mainly expect peers/helpers for something we use in multiple places, not just once)
There was a problem hiding this comment.
(I'm merging, expecting we'll come up with some unified solution for InstallPathsTestPeer, TypedNodesTestPeer, and InstTestHelper)
Stemming from #4331, trying to break apart InstallPaths class comments into three parts:
prefix_install_dirsFor code refactoring,
driver()andprefix()were only used by the install paths test. Rather than having a comment not to useprefix(), this instead extracts it out to a TestPeer model (which we have elsewhere withTypedNodesTestPeer, thus my choice in approaches).