Re: [PATCH] fix up external acl type dumping

On Fri, Jun 15, 2012 at 4:23 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 06/14/2012 03:06 AM, Robert Collins wrote:
>
>> +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \
>> +            case _external_acl_format::EXT_ACL_##a: \
>> +                storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \
>> +                break
>
> I do not see Squid using __VA_ARGS__ currently. Are you sure it is
> portable, especially in the ## context? If you have not tested this
> outside Linux, are you OK with us pulling out this patch if it breaks
> the build?

Yes, that would be fine. I'm mainly fixing this because I noticed it
in passing: it has no concrete effect on me.

> If you are not sure, you can rework the macro to always use a single
> argument instead. The calls without a parameter in the format can add
> one and use an empty string (or you can have two macros).
>
>
> I do not like how this macro butchers the format code names. I could
> borderline agree with stripping the namespace, but stripping "EXT_ACL_"
> prefix seems excessive. The prefix itself looks excessive (because the
> namespace already has "external_acl" in it!) but that is a different issue.
>
> I would prefer that the macro takes a real code name constant, without
> mutilating the name. There is an existing similar macro that does the
> same kind of mutilation, but there it is kind of justified because the
> name suffix is actually used.
>
> The true solution to this mess is to fix the names themselves, I guess.

I think we should make external acls a class, no base class, and ditch
the whole big case statement etc, and we can lose the macro at the
same time. Thats a rather bigger change, and I wanted to fix the
defect in the first instance.

-Rob
Received on Fri Jun 15 2012 - 07:34:12 MDT