Initial support for engines library#2367
Conversation
|
This PR is an evolution of the previous attempts in PR #1548, with input from discussions in PR #1635. The consensus AFAICT is that we should have a library for this logic but not a specific namespace. So this PR sets up the library using the model previously used by |
rousskov
left a comment
There was a problem hiding this comment.
I plan to come back to this PR after the backlog is cleared.
| #include "adaptation/ecap/Host.h" | ||
| #include "adaptation/ecap/ServiceRep.h" | ||
| #include "adaptation/ecap/XactionRep.h" | ||
| #include "AsyncEngine.h" |
There was a problem hiding this comment.
Please do not remove a header declaring a class that is used by the including code, even if some other included header happens to include that class declaration today. The same concern probably applies to some other modified files as well.
|
|
||
| include $(top_srcdir)/src/Common.am | ||
|
|
||
| noinst_LTLIBRARIES = libengines.la |
There was a problem hiding this comment.
PR description: Squid main operational loop uses a set of Engines that perform various roles. This creates a library for the code underlying that logic.
Amos: This PR is an evolution of the previous attempts in PR #1548, with input from discussions in PR #1635. The consensus AFAICT is that we should have a library for this logic but not a specific namespace.
It is not clear to me what "underlying that logic" and "this logic" refers to here, so I cannot confirm that such a consensus exists. Creating a library consisting of one header file feels wrong, so I assume that this PR implies that some other code (containing some additional "logic") will be added to that library. That "other code" will help flesh out the actual library scope. We should decide what else should go into that library before finalizing and merging this PR. We do not need to agree on a comprehensive list, but we need to agree on more than just one header file. The comment quoted below may be a step towards making that decision:
Amos: the
libengines.lasupplies a shared hierarchy API class (thisAsyncEngine) and perhaps the MainLoop/EventLoop code
Grouping AsyncEngine API and EventLoop code together makes sense, especially since that code is used by multiple executables today, but that group/library should not be called "engines" -- it provides no engines! A name like "MainLoop" or "EventLoop" may work (and would not preclude adding more engines to that library if we decide that those additions are a good idea).
| #ifndef SQUID_SRC_EVENTLOOP_H | ||
| #define SQUID_SRC_EVENTLOOP_H | ||
|
|
||
| #include "engines/AsyncEngine.h" |
There was a problem hiding this comment.
Please do not pull class declarations where a forward declaration would suffice, especially when existing code already uses a forward declaration (that current PR branch removes below). Instead, please add a forward.h file for the new library and start accumulating forward declarations there.
Squid main operational loop uses a set of Engines that perform
various roles.
This creates a library for the code underlying that logic.