[squid-dev] [PATCH] Older response must not update

[Alex Rousskov]

On 08/25/2016 08:18 AM, Eduard Bagdasaryan wrote:
> 2016-08-24 18:20 GMT+03:00 Amos Jeffries <squid3 at treenet.co.nz>:
> 
>> in src/LogTags.cc:
>> * instead of adding new enum entry please extend LogTags with a new bool
>> flag and the c_str() to append the "IGNORED" when that flag is true.

> Added a new LogTags::ignored for this.

The naming of things in logging code is exceptionally poor so please do
not feel upset that you got this wrong. The "_IGNORED" flag Amos is
talking about should go into LogTags::Errors.

I recommend renaming and re-documenting that subclass:

/// Things that may happen to a transaction while it is being
/// processed according to its LOG_* category. Logged as _SUFFIX(es).
/// Unlike LOG_* categories, these flags may not be mutually exclusive.
class Flags
{
...
   bool ignored; ///< _IGNORED: the response was not used for anything
   bool timedout; ///< _TIMEDOUT: terminated due to a lifetime or I/O
timeout
   bool aborted; ///< _ABORTED: other abnormal termination (e.g., I/O error)
} flags;

This does not fix all the problems, but reduces the confusion
considerably IMHO.


> I wonder whether the
> LogTags assignment operator should copy this member too.

Which assignment operator?

* The compiler-generated assignment operator will copy all members
automatically, of course.

* The explicitly-defined LogTags_ot conversion assignment operator is
already buggy or dangerous. It should be removed. Whether it was meant
to clear the flags, I do not know, unfortunately. I recommend leaving it
as is because fixing it is out of scope and may not be trivial. Adding
an "XXX: this operator does not reset flags" and "TODO: either replace
with a category-only setter or remove" comments would be appropriate.


HTH,

Alex.