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

On 09/18/2012 10:36 AM, Kinkie wrote:
>>> - 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).
>
> The names reflect my attempt at highlighting that these flags are not
> consistent: some seem to be markers that something happened. Some seem
> to be requests to something at a later processing stage. The names are
> my attempt at disambiguating those two cases.

And some are both cases? I doubt we should try to convey so much
information in a flag name because nobody will notice what we are trying
to do but many will be confused by apparent inconsistencies.

>> 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?
>
> One possible benefit I saw was to clarify when things are set and when
> they are tested.

An assignment versus an if statement clarifies that enough IMO. In fact,
a method call versus a method call provides a far less obvious visual
distinction.

>> 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).
>
> That's a possible optimization. I wonder however what happens if
> nothing is specified in the constructor list.

Nothing happens (except for default data member constructors, if any).
The generated default constructor does nothing (but execute data member
constructors, if any).

> Maybe the
> initializations are silently done? Or a memset is done? I suspect it
> is undefined in the standard.

It is very well defined. In general, you do not pay for what you do not
use in C++, in part to achieve partial backwards compatibility with C.
There is no magic :-).

>> 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).
>
> I would like to explain a little bit more about this, as it is another
> bit that was irking me.
> Some of the processing code is full of
> struct {
> #if somefeature
> int somevalue;
> #endif
> //...
> };
> int somefunction() {
> #if somefeature
> process(struct.somevalue);
> #endif
> }
>
> This XFF change is an attempt to spark discussion on a pattern for
> getting rid of this spaghetti (in a similar way like what happened
> with the initializers).

Strictly speaking, #if-statements do not create spaghetti code, but I
think I know what you mean. If we can remove many #if-statements by
always providing a few methods, we should provide those methods (and
make them no-op when needed using #if-statements). There are many gray
areas where the decision is not obvious, unfortunately.

> The proposal is to sacrifice a bit of
> space-efficiency in the data structures and using #ifdefs that cause
> NOPs in the functions, hopefully containing fetures enabling in the
> called code, freeing the caller from having to care.

Right. And whether the data member needs to be #ifdefed also depends on
several factors. Complex or large members and members requiring complex
or slow initialization should be #ifdefed (but we sometimes provide a
whole no-op class to handle such cases better!).

> Whether the effort is useful, I'd like us to spend some time discussing on.

I doubt we will find a one-size-fits-all approach, unfortunately.

>>> + 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.
>
> Old code didn't care. It's another step at what I was trying to
> obtained. The invariant is: if FOLLOW_X_FORWARDED_FOR is unset,
> doneFollowXForwardedFor should always return true.

Whatever the explanation is, the code looks wrong because the "false ||"
part does not do anything. It should be removed or replaced. Perhaps you
meant something like

    doneFollowXForwardedFor = !FOLLOW_X_FORWARDED_FOR;

?

>> 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.
>
> I'm not sure I understand what you mean here.. I'm generally sticking
> to trunk so far.

Think of where the committed trunk code will end up being: Current trunk
is [future] v3.3. After v3.3 has been branched, the trunk becomes
[future] v3.4.

Please do not commit a partial patch now if your next partial commit
will probably happen _after_ v3.3 has been branched. If you are not sure
you have enough time to finish this project before v3.3 is branched,
then commit all partial changes after v3.3 has been branched (but before
v3.4 has been branched). Does this clarify?

Thank you,

Alex.
Received on Tue Sep 18 2012 - 18:37:34 MDT