> On Jun 28, 2017, at 2:19 AM, Benjamin Mahler <bmah...@apache.org> wrote: > > Thanks James! As you said, removing Process implementations from the > headers is the existing practice, but we need to do a sweep to enforce this > consistently. Folks could work on this sweep today to make progress on the > 3 benefits you outlined. > > This proposal to me seems to just be: > > (1) When needed for testing, whether to expose the Process declaration in > its own foo_process.hpp header, rather than within foo.hpp. > (2) whether to name the .cpp as foo_process.cpp rather than foo.cpp. > > I'm not sure if I like (2), instead of keeping the .cpp named foo.cpp. > Consider the case where there is no foo_process.hpp (not needed for > testing), then you just have foo.hpp and foo_process.cpp. Or consider the > case where a user is looking for the implementation of limiter.hpp, they > have to know to look for limiter_process.cpp rather than limiter.cpp (but > only when a Process is involved!). Seems unfortunate?
I'm OK with putting both Foo and FooProcess in foo.cpp. > For Mesos, (1) sounds good, but I'm not sure if libprocess should be > exposing the foo_process.hpp header in the public includes alongside the > foo.hpp header. Because then libprocess users are assuming our particular > implementation of the interface. I think for the libprocess testing > purposes, we probably want the *_process.hpp header to be within the src/ > directory? There are 2 options for libprocess. Put the internal headers in the src/ directory, or keep them in include/ but don't install them (use noinst_HEADERS). The former gives better protection against accidentally consuming the internal headers from Mesos. > On Sat, Jun 24, 2017 at 8:23 AM, James Peach <jor...@gmail.com> wrote: > >> Hi all, >> >> There is a common Mesos pattern where a subsystem is implemented by a >> facade class that forwards calls to an internal Process class, eg. Fetcher >> and FetcherProcess, or zookeeper::Group and zookeeper::GroupProcess. Since >> the Process is an internal implementation detail, I'd like to propose that >> we adopt a general policy that it should not be exposed in the primary >> header file. This has the following benefits: >> >> - reduces the number of symbols exposed to clients including the primary >> header file >> - reduces the number of header files needed in the primary header file >> - reduces the number of rebuilt dependencies when the process >> implementation changes >> >> Although each individual case of this practice may not improve build >> times, I think it is likely that over time, consistent application of this >> will help. >> >> In many cases, when FooProcess is only used by Foo, both the declaration >> and definitions of Foo can be inlined into "foo.cpp", which is already our >> common practice. If the implementation of the Process class is needed >> outside the facade (eg. for testing), the pattern I would propose is: >> >> foo.hpp - Primary API for Foo, forward declares FooProcess >> foo_process.hpp - Declarations for FooProcess >> foo_process.cpp - Definitions of FooProcess >> >> The "checks/checker.hpp" interface almost follows this pattern, but gives >> up the build benefits by including "checker_process.hpp" in "checker.hpp". >> This should be simple to fix however. >> >> thanks, >> James
