Re: [PATCH] Comm Write cleanups

On 11/11/2010 07:55 AM, Amos Jeffries wrote:
> Updated version of earlier patch. This one works again after the client
> write pools merged.
>
> * creates namespace Comm::IO for the read and write functionality to exist.

Do we really need the Io sub-namespace? What Comm[unication] namespace
parts are not about I/O? For example, all code already in src/comm/, and
pretty much everything in comm.cc is also about I/O. If you are not
sure, let's drop the Io namespace from Comm.

> +}; // namespace Io
> +}; // namespace Comm

Extra semicolon after '}'.

You may want to fix others as well but such changes would be out of this
patch scope, of course:

src/auth/Config.h:}; // namespace Auth
src/comm/ListenStateData.h:}; // namespace Comm
src/esi/Module.h:}; // namespace Esi
src/eui/Config.h:}; // namespace Eui
src/eui/Eui48.h:}; // namespace Eui
src/eui/Eui64.h:}; // namespace Eui
src/fs/Module.h:}; // namespace Fs
src/ident/Config.h:}; // namespace Ident
src/ident/Ident.cc:}; // namespace Ident
src/ip/Address.h:}; // namespace Ip
src/ip/Intercept.h:}; // namespace Ip
src/ip/QosConfig.h:}; // namespace Qos
src/ip/QosConfig.h:}; // namespace Ip
src/ip/tools.h:}; // namespace Ip
src/ipc/StrandCoords.h:}; // namespace Ipc
src/log/Config.h:}; // namespace Log
src/SquidMath.h:}; // namespace Math

> * The comm_write() functions are moved into that scope as
> Comm::IO::Write() and only accept AsyncCall now.
>
> * commio_* functions are all moved to methods of a new
> Comm::Io::Callback object. Which represents either a read or a write
> callback event waiting to happen. Old wrapper versions of the functions
> have been removed.
>
> * The fdc_table of pending read and write callbacks has been moved into
> the Comm::IO scope with (the name iocb_table) and should be considered
> private. For now the COMMIO_*_CB() macros are retained to produce a
> pointer to a callback object in this table.
>
> * libcomm-listener.la has been renamed to libcomm.la

* Should comm_err_t and its members be moved inside Comm namespace,
dropping their comm prefixes? We can use transitional #defines to avoid
widespread changes if that is what you are trying to prevent. It is kind
of wrong to move comm_err_t to comm/ but leave it outside the Comm
namespace.

> This is not expected to produce any functionality changes.

Overall, this looks good to me although I could not verify that there
are no functionality changes due to code relocation (I have heard that
only git can track those).

Thank you,

Alex.
Received on Wed Nov 17 2010 - 22:41:11 MST