[squid-dev] [PATCH] improve CharacterSet

[Amos Jeffries]

On 27/12/2015 10:37 p.m., Kinkie wrote:
> On Sun, Dec 27, 2015 at 2:50 AM, Alex Rousskov
> <rousskov at measurement-factory.com> wrote:
>> On 12/26/2015 09:03 AM, Kinkie wrote:
>>> Hi all,
>>>   the attached patch improves CharacterSet by adding a subtraction
>>> operator, equality test and associated unit tests.
>>
> 
> Hi Alex, thanks for taking the time.
> 
>> Perhaps this is different in C++11, but IIRC we are supposed to:
>>
>> 1. declare non-modifying operators such as "-" outside the class
> 
> That's typical but not mandatory. Changing all to reflect
> 
>> 2. declare an inequality operator "!=" when declaring an equality operator
> 
> If unequality is not tested.. Still, adding it is trivial, so I'm doing that.
> 
>> For #1, you can claim code consistency as an excuse because the "+"
>> operator was already misplaced, but since no existing or proposed code
>> uses the new operators (AFAIK), it seems like it would be a good idea to
>> fix the wrong location before (or while) adding the new [unused] ones.
> 
> Done.
> 


AFAIK that guideline labeling these as 'misplaced' is all about whether
the global scope has something else where an implicit cast does bad
things. If so then global scope operator is effectively mandatory to
prevent the cast. Otherwise class scope operator works fine, and
preferring class scope helps reduce such clashes in future that might
cast to/from a CharacterSet.

I dont have any preference either way. But it does appear clear that we
should stick to doing it one way or the other consistently.

Shall we pick one and write it into the guidelines?

>> /// set subtraction: remove all characters that are also in src
>>
>>
>> "src" is a weird name for the parameter subtraction operator argument.
>> You can cite code consistency as an excuse but consider using "other" or
>> perhaps something like "banned" instead.
> 
> rhs? that's fairly common for operators

Either IMO.

> 
>>>      /// return a new CharacterSet containing the union of two sets, labeled as the first argument
>>>      CharacterSet operator +(const CharacterSet &src) const;
>> ...
>>> +    /// return a new CharacterSet containing the set of characters in the first but not in the second
>>> +    CharacterSet operator -(const CharacterSet &src) const;
>>
>>
>> It is funny that you preserved many bad things but decided to drop
>> documentation about the label. Please add it.
>>
>> Please note that neither the old operator "+" nor the new operator "-"
>> have two [explicit] arguments [until they are moved outside the class as
>> discussed in #1 above] so the comment using words "first" and "second"
>> is rather confusing!
> 
> Moved outside class; renamed parameters to lhs and rhs, reworked
> doxygen comment.
> 


I think we should be adding a remove() method. And implementing the +/-
operators using a loop calling add() and remove().


>>> +static
>>> +std::ostream& operator<< (std::ostream &s, const CharacterSet &c)
>>> +{
>>> +    s << "CharacterSet(" << c.name << ')';
>>> +    return s;
>>> +}
>>> +
>>
>> Perhaps this should be provided in the CharacterSet header, for all to
>> use? You would have to split the definition from the declaration to
>> avoid dragging iostream into the header, but this operator feels useful
>> to me, even in its current "primitive" form.
> 
> I've thought about it, and decided not to, as it'd pull in iostream to
> all users of CharacterSet.
> This is mostly meant as a convenience to allow using
> CPPUNIT_ASSERT_EQUAL which is more telling that CPPUNIT_ASSERT but
> requires being able to print arguments.
> 

It is useful for debugs() as well.

Does using iosfwd and explicitly using inline attribute avoid iostream
in the .cc where the operator is unused?

Amos