Trivial: Rename ServerHandlerImp to ServerHandler

[scottschurr]

High Level Overview of Change

There's only one implementation of ServerHandler. Let's call it what it is.

As part of some historical hack, this was part of our code base:

namespace ripple {
using ServerHandler = ServerHandlerImp;
...
}

There was no other ServerHandler definition, despite the existence of a header suggesting that there was.

Context of Change

I accidentally stumbled across this weirdness in our code base as part of a code review I was doing. I almost asked for the change to be incorporated into that pull request. But the problem was not introduced by that pull request, so that seemed an unfair burden.

Therefore I'm submitting this as a (trivial) stand-alone change.

The point of the change is two fold:

1. Let's not give something two names unless it serves a purpose. Having two names, ServerHandler and ServerHandlerImp, for the same thing helped no one.
2. We should prefer simpler names over complicated names. When collapsing to a single name, ServerHandler is simpler than ServerHandlerImp. Let's use the simple name.

Type of Change

• Refactor (non-breaking change that only restructures code)

Note that, because of the way git does diffs, the changes look worse than they are. There was an old (small) file called

src/ripple/rpc/ServerHandler.h

All of the contents of

src/rippled/rpc/impl/ServerHandlerImp.h

were merged into ServerHandler.h. So ServerHandler.h looks like it has a lot of changes. But it's just that it absorbed the contents of ServerHandlerImp.h, which were the important parts.

[drlongle]

I also came across this hack sometimes ago. Thanks for fixing it.

[ckeshava]

I'm curious about using other types as a private tag in constructors. Can we use an int or a char or some static variable (in the private access specifier of class) as a tag?

I know that this struct is being passed with const& and may not incur high overhead.

[scottschurr]

First it's worth noting that we're pushing C++ to do something it's not naturally inclined to do. We're trying to limit who is allowed to construct this type.

Let's look at how ServerHandlerCreator is being used. Here's the implementation of make_ServerHandler :

std::unique_ptr<ServerHandler>
make_ServerHandler(
    Application& app,
    boost::asio::io_service& io_service,
    JobQueue& jobQueue,
    NetworkOPs& networkOPs,
     Resource::Manager& resourceManager,
     CollectorManager& cm)
 {
     return std::make_unique<ServerHandler>(
         ServerHandler::ServerHandlerCreator(),
         app,
         io_service,
         jobQueue,
         networkOPs,
         resourceManager,
         cm);
 }

Note that make_ServerHandler() does not return a ServerHandler or even a reference to a ServerHandler. It returns a std::unique_ptr<ServerHandler>. And it does so by calling std::make_unique(). This is awkward because somewhere in the depths of std::make_unique the constructor of ServerHandler will be invoked. We have no idea where in the standard library that invocation will occur. So we can't declare the part of the standard library as a friend. And even if we could, different standard library implementations will do it differently.

The work around is to pass a unique type (e.g. ServerHandlerCreator) to the constructor of ServerHandler. We can make sure that only make_ServerHandler() has access to that type by making the type private and then declaring make_ServerHandler() a friend. Once make_ServerHandler() has constructed that unique type, the type can be passed around to anyone.

In effect we've made a way for only make_serverHandler() to give permission to std::make_unique() to construct a ServerHandler.

We have to use a type for this. Parameter lists are sensitive to types. If we used an int or a char to grant this permission, well, anyone can pass an int or a char. If we make the permission thing a type, then the compiler can assure us that the correct type is being passed as a parameter.

Make sense?

[ckeshava]

okay, that makes a lot of sense. Yeah, we can't impersonate types, understood.

[ckeshava]

Can you please point me to resources about this make_XYX design pattern? I've read about the factory design pattern and I'm trying to understand it better.

1. Why not make this function a member of the class, instead of declaring it as a friend?
2. Why do we need this function? It appears to be calling a constructor of the class (without any additional preprocessing). Can't we make the said constructor public, thereby reducing the number of indirections.

I'm new to this design pattern, I'd like to learn more about it.

[scottschurr]

1. Why not make this function a member of the class, instead of declaring it as a friend?

It could be a static member of the class. I'm not sure why a free function was preferred over a static member in this case. It's probably for uniformity. There are quite a number of other make_ factory functions around the code base. They are easier for developers to use and think about if they have similar interfaces and behavior.

2. Why do we need this function?

Factory functions can have a number of different purposes. In this case it's because we want to prevent anyone from constructing a ServerHandler that is not held by a std::unique_ptr<ServerHandler>. If we made the ServerHandler constructor public, then anyone could create one. So, in this case, we give the factory the permission to allow the construction and trust that the factory will do the right thing with the unique_ptr. And we don't have to trust very hard, since the implementation for make_ServerHandler() is in the implementation file for ServerHandler.

Factory functions can be used for other purposes as well. For example, the Meyer's Singleton is (in effect) a factory function: https://en.wikipedia.org/wiki/Singleton_pattern

The (ancient) Gang of Four Design Patterns book has a chapter on Factory Method.

Factory functions really just solve the problem that sometimes a simple constructor can't do quite enough. The factory function can do the extra work that is required.

[ckeshava]

hmm okay. I'll check out these resources 👍

[ckeshava]

Is it okay to leave an extra line in the include headers? I hoped the clang-format command took care of extra whitespace lines.

My apologies, I don't mean to be anal about coding style here, just wanted to bring it to your attention.

[scottschurr]

This line of whitespace is allowed by clang-format and actually serves a purpose.

We ask clang-format to alphabetize our #include lines. That helps make it more obvious when the same file is included twice. Sounds silly, but it happens when there are lots of #includes.

But there are situations where we don't want things alphabetized. Here is one of them. In the book Large Scale C++ Software Design, John Lakos recommends that in an implementation file the first file that should be included is the header file for that implementation. Here's the principle he gives for that rule (pg 111 of the edition I own):

Latent usage errors can be avoided by ensuring that the .h file of a component parses by itself--without externally-provided declarations or definitions.

By placing that header first the compiler can show us that the header itself includes all of the headers it needs to be complete.

Suppose that a header relied in <vector> but did not #include <vector> itself or through some other include. As long as that header is included elsewhere after <vector> has already been included, then everything builds and the problem is hidden.

If there's at least one place where the header is included first, then the compiler can assure us that the header contains everything it needs to stand alone. Mr. Lakos suggests that one place can be the implementation file. We follow his advice.

So, back to the line of whitespace.

clang-format only alphabetizes contiguous #includes, without intervening whitespace. So by putting in the whitespace we can keep clang-format from pushing this #include out of its top-most position.

We may also use a line of white space between ripple #includes and system #includes. We want the more-local headers to be included first, followed by more general headers. That line of whitespace prevents clang-format from stirring them all together.

[ckeshava]

this is really helpful, thanks for letting me know about the convention 👍
To paraphrase what I understood : It is good to include the header file of an implementation at the beginning of the implementation file. This helps in providing a self-contained list of header files for the implementation right?

I understood the dependency ordering of the #include's in the third paragraph from the end 👍

[intelliot]

Note: the current plan is to hold this until after 1.11 stable has been released