[squid-dev] [PATCH] Remove ServerOptions "partial copy" copy constructor

[Alex Rousskov]

On 04/09/2016 11:03 PM, Amos Jeffries wrote:
> On 8/04/2016 2:57 a.m., Alex Rousskov wrote:
>> On 04/07/2016 03:22 AM, Amos Jeffries wrote:
>>> On 7/04/2016 6:10 p.m., Alex Rousskov wrote:
>>>>     The attached patch removes broken and, AFAICT, unused "partial copy"
>>>> ServerOptions copy constructor.


>>> A (full) copy-constructor is needed on some systems for the clear()
>>> method implementation to build.


>> AFAICT, the clear() method needs an assignment operator rather than a
>> copy constructor.
>>
>> Is there something wrong with the full copy constructor generated by
>> default?


> IIRC there the default was none (implicit =delete) but some of the
> operators from AnyP::CfgPort try to make use of it.

In the current code, the "default" copy constructor for ServerOptions
would be not none/=delete, but a member-wise copy. Same for the
assignment operator. Perhaps the code has changed since when the
opposite was true.


> I dont recall which system or compiler version specifically sorry. But
> CfgPort should only have copy-construct actions done on it for
> split-stack OS such as OpenBSD. So maybe it only shows up there.

FWIW, the patch does not really remove a constructor so it should not
affect compilation. It just lets the compiler generate the [correct]
version of that constructor. My concern was that perhaps the
staticContext member was manually excluded from being copied on purpose
rather than by accident. The patched code copies all ServerOptions
members, including staticContext, and that is the change I am worried about.


> IIRC it was originally found in the BuildFarm testing so if this patch
> passes the current BuildFarm matrix it can go in. And/or just apply and
> add a proper ctor later if needed.


> (I'll tentatively +1 this as-is if you want to do the latter).

Committed to trunk (r14637). If I notice any related build failures, I
will try to address them.


Thank you,

Alex.