[squid-dev] [PATCH] improve CharacterSet

[Alex Rousskov]

On 12/27/2015 06:01 PM, Amos Jeffries wrote:
> On 27/12/2015 10:37 p.m., Kinkie wrote:
>> On Sun, Dec 27, 2015 at 2:50 AM, Alex Rousskov wrote:
>>> 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.


> AFAIK that guideline labeling these as 'misplaced' is all about whether
> the global scope has something else where an implicit cast does bad
> things. 

s/bad/good/

If global implicit casts are possible they must be good (desirable,
intentional, welcomed, etc). If global implicit casts are possible and
are bad, then they have to be removed, of course, but that would mean
that the problem is elsewhere, and, after that other problem is fixed,
any remaining global implicit casts are the good ones.


> Shall we pick one and write it into the guidelines?

Yes, we should follow the C++ best practice of declaring non-modifying
operators such as "-" outside the class. I am not 100% we can or should
re-document all C++ best practices "locally", but please do not
misinterpret my doubts as an objection to attempting to do that.



>>>> +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.

Exactly.


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

I hope that iosfwd avoids most iostream overheads. That is why I
suggested avoiding iostream in the CharacterSet header.


HTH,

Alex.