Sounds good to me, I also like putting the internal limiter_process.hpp (if needed for testing) in the src folder (note that we already have internal headers there).
Thanks James! Looking forward to having this tidied up. Let me know if you need any assistance wrangling reviewers :) On Wed, Jun 28, 2017 at 8:54 PM James Peach <jor...@gmail.com> wrote: > > > 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 > >
