Re: [Patch] Fix ONE_DIRECTION undef warnings.

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/30/2014 04:06 PM, Roland McGrath wrote:
>> That sounds like you're against the default header design. Could you clarify
>> your position on that?
> 
> I am against anything that is prone to typos introducing silent errors when
> we can come up with an alternative solution that is not a worse maintenance
> burden in other regards.  That is an abstract position and that's all I
> have because I don't have a specific alternative proposal for
> iconv/skeleton.c at the moment.  I'm hoping to encourage other folks like
> you to explore new alternatives that might satisfy my constraints better
> than what we've discussed so far.

Awesome. I'll keep proposing things until you stop complaining :-)

>> (c) Uses that need to override defaults do so like this.
>>
>> #undef FOO
>> #define FOO 1
> 
> This remains typo-prone in the sense that if FOO is the name in the macro
> API but an overriding use is instead written as:
> 
> 	#undef	FO0
> 	#define FO0 1
> 
> then the default value of FOO reigns and this unused FO0 macro is never
> diagnosed.

Whose solution forbids defaults. I'm not against that.

> With this fundamental style of "macro API" there is an unavoidable
> trade-off between avoiding this possibility and avoiding duplicative
> default definitions in many places.  (The actual risk of error in the
> duplicative situation can be avoided by not duplicating the default value
> but instead duplicating "#define FOO FOO_DEFAULT" or suchlike.)  Neither
> situation is particularly appealing.

I prefer avoiding ABI mistakes to avoiding duplication. My opinion is
that the duplication is simply the way the "macro API" works, you define
the macros for your implementation, and you define all of them.

> In general, we avoid these problems best by avoiding the "macro API"
> pattern whenever we can do something else that is cleaner without
> sacrificing other important concerns (like performance).  Sometimes that's
> hard, as is probably the case for iconv/skeleton.c.  An example of avoiding
> it entirely would be using C variables instead, where:
> 	static const sometype name_with_typo = 23;
> would generate an unused-variable varible warning if name_with_typo is not
> the name actually used.  In the absence of other horrors like C++ with
> template tricks, there probably isn't a way to both use this style and have
> defaults not duplicated in each user.  With recent compilers, this style is
> OK for performance concerns because the compiler optimizes away the
> variable and constant-folds conditionals and calculations that use it.

Agreed.
 
> Another example of how the typo-prone pattern can be avoided while sticking
> to a "macro API" is to replace the "user defines a bunch of constants" API
> with something where the user does:
> 
> 	#define THING_WITH_PARAMS THING (2, 3, 4, 5)
> 
> i.e., to replace:
> 
> 	#define THING_PARAM1	2
> 	#define THING_PARAM2	3
> 	#define THING_PARAM3	4
> 	#define THING_PARAM4	5
> 
> To avoid the duplicated-defaults issue that can be:
> 
> 	#define THING_WITH_PARAMS THING (DEFAULT_PARAM1, 3, 4, 5)
> 
> But of course that has other maintainability issues fairly similar to the
> current style requiring each user to define a macro for each parameter
> (pervasive mechanical changes when a parameter is added, removed, or
> renamed) and ones of its own (positional parameters are always hard to
> interpret by eyeball).

On top of that maintainers will likely just do:

 	#define THING_PARAM1	2
 	#define THING_PARAM2	3
 	#define THING_PARAM3	4
 	#define THING_PARAM4	5
 	#define THING_WITH_PARAMS THING (THING_PARAM1, THING_PARAM2, THING_PARAM4, THING_PARAM4)

Precisely because positional parameters are harder to read.

Then they will typo the params into:

 	#define THING_PARAM1	2
 	#define THING_PARAM2	3
 	#define THING_PARAM3	4
 	#define THING_PARAM4	5
 	#define THING_WITH_PARAMS THING (THING_PARAM1, TH1NG_PARAM2, THING_PARAM4, THING_PARAM4)


> So, like I said, I have not come up with a particular alternative that is a
> great fit for the iconv/skeleton.c case.  But I hope these examples suggest
> the kind of general rethinking that I hope we'll do when trying to make any
> particular case along these lines cleaner and easier to maintain.

You've only underscored the fact that the solution we should take as a first
round is to define ONE_DIRECTION is all the sources files that include
iconv/skeleton.c. It's the least error prone and is a full definition of the
"macro API" in each use.

The only alternative I can think of is a two step process as you suggest.

Default header does:

	#define DEFAULT_FOO	0

Override does:

	#define FOO DEFAULT_FOO

Use does:

	#if FOO
	...
	#endiff

Which makes it typo-prone because the user *must* define
the final value of the name.

(4) Incorrect override of default.

A typo in the override of the default results in a -Wundef failure.

e.g.

#define FO0 DEFAULT_FOO

or

#define FOO DEFAULT_FO0

Result in compiler errors. The first one at the first undefined
use of FOO and the latter when the header defining FOO is included
(since DEFAULT_FO0 doesn't exist).

Does that work?

If it does I'll put it on a wiki page and document best practice
to help all the developers trying to fix -Wundef warnings.

Cheers,
Carlos.