Re: [RFC] Interim review: typedefs.h / protos.h splitting

On 09/17/2012 07:47 AM, Kinkie wrote:

> Attached you'll find an interim patch aiming at replicating for
> typedefs.h and protos.h the same work I've done for protos.h.
>
> It
> - separates structures declarattions into own header files

Please try to limit the number of #includes in header files. If a header
does not dereference a class, it does not need to include the
corresponding class declaration in most cases. For example:

> +#include "err_type.h"
> +#include "acl/AclNameList.h"
> +
> +class AclDenyInfoList {
> +public:
> + err_type err_page_id;
> + char *err_page_name;
> + AclNameList *acl_list;
> + AclDenyInfoList *next;
> +};

The acl/AclNameList.h #include can be replaced with a forward declaration:

    class AclNameList;

For more complex cases and especially for groups of commonly used
related classes, a corresponding "forward.h" header or equivalent is
appropriate.

This approach will reduce the number of dependencies between .cc files,
often significantly reducing compilation time during active development.

> - transform typedef structs (and plain structs) into classes

I wonder whether it is a good idea to add a "currently a POD" or a
similar comment to the description of these from-struct classes that
usually lack proper constructors, destructors, etc.?

Before these changes, if I saw a "struct", I knew that it most likely
lacked those C++ properties. Now, it would be more difficult to
distinguish those cases because the "struct" hint is gone.

Not every POD needs to be converted into a "real" class, of course, but
we need to be aware what kind of object we are dealing with and a "POD"
label may be helpful.

> - renames some types to CamelCase convention
> - adds some (for now empty placeholders) code files for c++-ification
> of classes (e.g. CacheMgrPasswd, CustomLog)

> - implements the full set of getters/setters for class RequestFlags -
> the method names are certainly improveable, advice is sought

I recommend undoing changes that do nothing but wrap a boolean field in
two or three methods. They just add more ink and the naming is not
consistent across different fields (e.g., some fields are "set" others
are "marked"; some have default set values, others do not).

Cases are where some additional processing/logic is implemented (e.g.,
nocache_hack) should be wrapped, but why complicate processing for the
vast majority of fields?

If you do insist on wrapping everything, please use a consistent naming
approach. I would recommend having one getter and one setter, both with
the same name and no default arguments.

There are three other issues related to the RequestFlags change:

> RequestFlags():
> + nocache(false), ims(false), auth_(false), cachable(false),
> + hierarchical_(false), loopdetect(false), proxy_keepalive(false), proxying_(false),
> + refresh_(false), redirected(false), need_validation(false),
> + fail_on_validation_err(false), stale_if_hit(false), nocache_hack(false), accelerated_(false),
> + ignore_cc(false), intercepted_(false), hostVerified_(false), spoof_client_ip(false),
> + internal(false), internalclient(false), must_keepalive(false), connection_auth_wanted(false), connection_auth_disabled(false), connection_proxy_auth(false), pinned_(false),
> + canRePin_(false), authSent_(false), noDirect_(false), chunkedReply_(false),
> + streamError_(false), sslPeek_(false),
> + doneFollowXForwardedFor(!FOLLOW_X_FORWARDED_FOR),
> + sslBumped_(false), destinationIPLookedUp_(false), resetTCP_(false),
> + isRanged_(false)
> + {}

For this discussion, let's assume that we initialize
doneFollowXForwardedFor to false like other fields (and then set it in
the constructor body to a proper value or make a smart getter).

Can you check whether GCC can optimize the above into some form of
memset(0). I suspect it cannot, especially since correctly setting a
single bit probably requires fetching and preserving the value of nearby
bits. If I am right, I wonder whether we should use memset(0)
explicitly? It feels like we are adding a lot of operations to every
request where one operation would work just fine (I know some compilers
will even unwrap memset for small constant size values but I do not know
whether GCC does that).

Since the old code did the same kind of bit-by-bit initialization, you
can investigate this optimization later. However, I would still change
doneFollowXForwardedFor initialization to follow the pattern (if nothing
else, this not-false exception tempts others to add more initialization
exceptions, complicating future optimizations).

> + void clearDoneFollowXFF() {
> + /* do not allow clearing if FOLLOW_X_FORWARDED_FOR is unset */
> + doneFollowXForwardedFor = false || !FOLLOW_X_FORWARDED_FOR;
> + }

Something went wrong here. The "false ||" part does not do anything. I
failed to find the corresponding old code so I am not sure what this
should be.

BTW, I recommend using "Xff" spelling in CamelCase names instead of
all-caps "XFF". We already use that approach for protocol names (e.g.,
Icap and Ssl) and it look better IMO.

> - request->flags.accelerated = http->flags.accel;
> - request->flags.sslBumped = conn->switchedToHttps();
> - request->flags.canRePin = request->flags.sslBumped && conn->pinning.pinned;
> - request->flags.ignore_cc = conn->port->ignore_cc;
> + if (http->flags.accel)
> + request->flags.markAccelerated();
> + request->flags.setSslBumped(conn->switchedToHttps());
> + if (request->flags.sslBumped() && conn->pinning.pinned)
> + request->flags.allowRepinning();
> + if (conn->port->ignore_cc)
> + request->flags.ignoreCacheControl();

I do not think your patch should change unconditional setting of a flag
into a conditional setting of a flag. Looking at the patch alone, I
cannot tell whether the change is correct, but even if it is correct
now, the old code was safer/clearer because it had no conditions.

If we remove wrappers, the above changes will not be needed.

If we add wrappers, I think the code should still look condition-less,
like this:

    request->flags.accelerated(http->flags.accel);
    request->flags.sslBumped(conn->switchedToHttps());
    ...

> - changes some linkage from C to C++

Instead of replacing SQUIDCEXTERN function prefix with "extern", please
completely remove the linkage type. Ordinary functions have external
linkage by default in C++.

> I'd like to get some feedback before committing more effort to this;
> also an interim merge could be useful - the patch is already more than
> 10klines long.

I think you are on the right track and the changes are useful. Avoiding
changes outside this patch intended scope ("typedefs.h / protos.h
splitting") such as making flag settings conditional would help minimize
contention.

I am OK with committing some of the changes as long as they are
self-contained and complete. We should balance that with the number of
conflicts these commits will create (with all the renaming going on).
However, if you commit partial changes, please make sure that all major
changes are committed to one [future] Squid version (e.g., all in v3.3
or all in v3.4). This will minimize porting pains for pending patches.

Thank you,

Alex.
Received on Tue Sep 18 2012 - 15:52:39 MDT