Re: [PATCH] fix up external acl type dumping

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?

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.

Cheers,

Alex.
Received on Thu Jun 14 2012 - 16:23:25 MDT