Fix ONE_DIRECTION undef warnings.

  On Tue, 2014-04-29 at 17:14 -0400, Carlos O'Donell wrote:

> Lastly, I hope that we can all agree that:
> 
> #ifndef ONE_DIRECTION
> #define ONE_DIRECTION
> #endif
> 
> Isn't quite what we want since it silently accepts a particular
> configuration. We also didn't do a good job of documenting this
> particular default :-(


OK, Here is a new (partial) patch for the undef's involving ONE_DIRECTION.
It undoes my earlier patch and includes defines of ONE_DIRECTION in
gconv_simple.c, 8bit-generic.c, and 8bit-generic.c.  This does not fix all
the warnings involving ONE_DIRECTION but it gets rid of several hundred and
I would like to get this approved and checked in before attacking the rest.
Unfortunately, it looks like fixing the remaining 100 undefs will require
one line fixes to 100 different files in iconvdata all of which include
skeleton.c.

OK to check this in?

Steve Ellcey
sellcey@mips.com


2014-04-30  Steve Ellcey  <sellcey@mips.com>

	* intl/iconv/skeleton.c (ONE_DIRECTION): Remove define.
	* iconv/gconv_simple.c (ONE_DIRECTION): Define.
	* iconvdata/8bit-gap.c (ONE_DIRECTION): Ditto.
	* iconvdata/8bit-generic.c (ONE_DIRECTION): Ditto.
  

On 04/30/2014 01:08 PM, Steve Ellcey wrote:
> On Tue, 2014-04-29 at 17:14 -0400, Carlos O'Donell wrote:
> 
>> Lastly, I hope that we can all agree that:
>>
>> #ifndef ONE_DIRECTION
>> #define ONE_DIRECTION
>> #endif
>>
>> Isn't quite what we want since it silently accepts a particular
>> configuration. We also didn't do a good job of documenting this
>> particular default :-(
> 
> 
> OK, Here is a new (partial) patch for the undef's involving ONE_DIRECTION.
> It undoes my earlier patch and includes defines of ONE_DIRECTION in
> gconv_simple.c, 8bit-generic.c, and 8bit-generic.c.  This does not fix all
> the warnings involving ONE_DIRECTION but it gets rid of several hundred and
> I would like to get this approved and checked in before attacking the rest.
> Unfortunately, it looks like fixing the remaining 100 undefs will require
> one line fixes to 100 different files in iconvdata all of which include
> skeleton.c.
> 
> OK to check this in?
> 
> Steve Ellcey
> sellcey@mips.com
> 
> 
> 2014-04-30  Steve Ellcey  <sellcey@mips.com>
> 
> 	* intl/iconv/skeleton.c (ONE_DIRECTION): Remove define.
> 	* iconv/gconv_simple.c (ONE_DIRECTION): Define.
> 	* iconvdata/8bit-gap.c (ONE_DIRECTION): Ditto.
> 	* iconvdata/8bit-generic.c (ONE_DIRECTION): Ditto.

OK to checkin.

This looks good to me.

Steve, Please don't take my word for it, and if you think this entire
exercise is stupid please say so. However, I agree with Roland that there
is considerable value in fixing up these "macro as interface" issues to
the point where an undefined macro is an error. Particularly when it
comes to ABI issues that might go unnoticed like the Power 2.18 ABI
issue around librt which was fixed by accident.

I agree that fixing this requires mechanical addition of defining
ONE_DIRECTION in each converter as part of the API contract with
using the skeleton.c file. This is core cleanup work that will bear
fruit!

Cheers,
Carlos.
  

> I agree that fixing this requires mechanical addition of defining
> ONE_DIRECTION in each converter as part of the API contract with
> using the skeleton.c file. This is core cleanup work that will bear
> fruit!

It may be an indication the the "macro API" needs some more substantial
cleanup that makes it possible for the users of the API (definers of the
macros) to be less duplicative of each other.
  

On Wed, 2014-04-30 at 13:42 -0400, Carlos O'Donell wrote:

> Steve, Please don't take my word for it, and if you think this entire
> exercise is stupid please say so. However, I agree with Roland that there
> is considerable value in fixing up these "macro as interface" issues to
> the point where an undefined macro is an error. Particularly when it
> comes to ABI issues that might go unnoticed like the Power 2.18 ABI
> issue around librt which was fixed by accident.

Frustrating and annoying but not stupid.  If we could get there at zero
cost I would have no issues with the direction we are going.  My main
problem is with the amount of change needed to get there and about
whether anyone was actually working on the changes.

> I agree that fixing this requires mechanical addition of defining
> ONE_DIRECTION in each converter as part of the API contract with
> using the skeleton.c file. This is core cleanup work that will bear
> fruit!

Yes, I think (hope) ONE_DIRECTION is the worst case scenario that we
have.

Steve Ellcey
  

On 04/30/2014 01:56 PM, Roland McGrath wrote:
>> I agree that fixing this requires mechanical addition of defining
>> ONE_DIRECTION in each converter as part of the API contract with
>> using the skeleton.c file. This is core cleanup work that will bear
>> fruit!
> 
> It may be an indication the the "macro API" needs some more substantial
> cleanup that makes it possible for the users of the API (definers of the
> macros) to be less duplicative of each other.

We have 54 source files using this "macro API."

Alternatively it's an indication of the need for better refactoring tools.

As Steve suggested in another email it might make sense to have a single
header define "macro API" defaults, that make sense for 99% of the
configurations. It would make it easy to adjust them en-masse instead of
rafactoring the changes across the 54 source files that use it.

Cheers,
Carlos.
  

On 04/30/2014 01:57 PM, Steve Ellcey wrote:
> On Wed, 2014-04-30 at 13:42 -0400, Carlos O'Donell wrote:
> 
>> Steve, Please don't take my word for it, and if you think this entire
>> exercise is stupid please say so. However, I agree with Roland that there
>> is considerable value in fixing up these "macro as interface" issues to
>> the point where an undefined macro is an error. Particularly when it
>> comes to ABI issues that might go unnoticed like the Power 2.18 ABI
>> issue around librt which was fixed by accident.
> 
> Frustrating and annoying but not stupid.  If we could get there at zero
> cost I would have no issues with the direction we are going.  My main
> problem is with the amount of change needed to get there and about
> whether anyone was actually working on the changes.

I signed up for it and right now I'm tackling FEATURE_INDEX_1 in x86,
which Roland had some comments about.

I think everyone is pitching in to help, and I'm trying to review the
changes with an eye towards fixing them correctly.

I admit the ugly warnings are there as incentive to fix the issue :-)

>> I agree that fixing this requires mechanical addition of defining
>> ONE_DIRECTION in each converter as part of the API contract with
>> using the skeleton.c file. This is core cleanup work that will bear
>> fruit!
> 
> Yes, I think (hope) ONE_DIRECTION is the worst case scenario that we
> have.

See my other email where I acknowledge your general idea of a single
default header definition being potentially useful here now and in
the future.

Cheers,
Carlos.
  

On Wed, 2014-04-30 at 10:56 -0700, Roland McGrath wrote:
> > I agree that fixing this requires mechanical addition of defining
> > ONE_DIRECTION in each converter as part of the API contract with
> > using the skeleton.c file. This is core cleanup work that will bear
> > fruit!
> 
> It may be an indication the the "macro API" needs some more substantial
> cleanup that makes it possible for the users of the API (definers of the
> macros) to be less duplicative of each other.

I was wondering if there was some way to determine the correct value
of ONE_DIRECTION in skeleton.c based on other macros like FROM_DIRECTION
but I didn't see any way to do that.

Steve Ellcey
  

> As Steve suggested in another email it might make sense to have a single
> header define "macro API" defaults, that make sense for 99% of the
> configurations. It would make it easy to adjust them en-masse instead of
> rafactoring the changes across the 54 source files that use it.

The obvious way to do this makes all those source files places where typos
could go unnoticed.  We should think hard about ways to reduce that danger.
IMHO this is a stronger motivator than avoiding duplicate changes that can
be easily mechanized.  The former leads to silent bugs gone unnoticed,
which is a risk of real user harm that might not be noticed until much
later.  The latter leads to build errors/warnings that alert one early to
the need to do the annoying mechanically-duplicative change, which is a
certainty of immediate slight maintainer annoyance that happens immediately
to the maintainer who instigated the change.
  

On 04/30/2014 02:10 PM, Roland McGrath wrote:
>> As Steve suggested in another email it might make sense to have a single
>> header define "macro API" defaults, that make sense for 99% of the
>> configurations. It would make it easy to adjust them en-masse instead of
>> rafactoring the changes across the 54 source files that use it.
> 
> The obvious way to do this makes all those source files places where typos
> could go unnoticed.  We should think hard about ways to reduce that danger.
> IMHO this is a stronger motivator than avoiding duplicate changes that can
> be easily mechanized.  The former leads to silent bugs gone unnoticed,
> which is a risk of real user harm that might not be noticed until much
> later.  The latter leads to build errors/warnings that alert one early to
> the need to do the annoying mechanically-duplicative change, which is a
> certainty of immediate slight maintainer annoyance that happens immediately
> to the maintainer who instigated the change.

That sounds like you're against the default header design. Could you clarify
your position on that?

My thoughts on the matter are:

(a) The default header defines defaults e.g.

#define FOO 0

(b) The uses include it early.

#include <foo-defaults.h>

(c) Uses that need to override defaults do so like this.

#undef FOO
#define FOO 1

(d) All uses of FOO remain safe.

#if FOO
...
#endif

Common mistakes:

(1) Include is late.

#undef FOO
#define FOO 1
#include <foo-defaults.h>

Multiple definition error. Developer has to fix it immediately.

(2) Wrong default.

Can't be fixed.

If you specify the wrong value that's logical error we can't correct.

(3) Complicates include order.

It does.

In that it must be:

#include <foo-defaults.h>
#include <includes-and-overrides-foo-defaults.h>

Or it doesn't work and we have the similar includes problem as defined here:
https://sourceware.org/glibc/wiki/Synchronizing_Headers

If you aren't against default headers, is your statement to serve as a reminder
that the build should fail early and immediately and such implementations should
be chosen over those that done? I agree.

Cheers,
Carlos.
  

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

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

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.

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.

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

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.


Thanks,
Roland
  

On 4/30/2014 4: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.

In the <arch/chip.h> header for the Tilera architectures we provide macros defining characteristics of the chip like this:

/** Does the chip have an IPI shim? */
#define CHIP_HAS_IPI() 1

This requires code that uses it to do

#if CHIP_HAS_IPI()
...
#endif

Typos then generate a cryptic but uniformly fatal error: missing binary operator before token "(".  The style is aesthetically a bit awkward, but we've become used to it :-)
  

On 04/30/2014 04:14 PM, Chris Metcalf wrote:
> On 4/30/2014 4: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.
> 
> In the <arch/chip.h> header for the Tilera architectures we provide
> macros defining characteristics of the chip like this:
> 
> /** Does the chip have an IPI shim? */
> #define CHIP_HAS_IPI() 1
> 
> This requires code that uses it to do
> 
> #if CHIP_HAS_IPI()
> ...
> #endif
> 
> Typos then generate a cryptic but uniformly fatal error: missing
> binary operator before token "(".  The style is aesthetically a bit
> awkward, but we've become used to it :-)

It still doesn't solve the typo in the overriding implementation
providing the wrong override e.g.

#undef CHIP_HAS_1PI() 1
#define CHIP_HAS_1PI() 0

#if CHIP_HAS_IPI()
...
#endif

You would still get the default for CHIP_HAS_IPI of 1.

If there was no default you would get an error as you suggest.

Cheers,
Carlos.
  

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.
  

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

That's an entirely reasonable position.
It highlights how poor an API it is.

> On top of that maintainers will likely just do:
[...]
> Precisely because positional parameters are harder to read.
> 
> Then they will typo the params into:
[...]

Probably true.  It wasn't a particularly satisfying pattern to begin with.

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

I don't object to that.

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

I don't see how that is actually typo-prone at all.  A typo anywhere in
the #define line results in a -Wundef error (aside from writing
DEFAULT_BAR when that exists but DEFAULT_FOO is what you meant, which
does not seem so likely).  

Your following text makes me think you actually meant to write
"typo-proof" there.  That was pretty meta, dude.

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

This works, but it seems rather cumbersome.  I'd say "best practice" is
to avoid macro APIs altogether.  When doing so would violate some other
best practice such as avoiding code duplication, then one should craft a
scheme that optimizes for the characteristics we like as best one can in
the particular case.  For cases that have fewer parameters than the
iconv/skeleton.c case, some more concise scheme might have all the
desireable characteristics.  It seems most important to document those
characteristics we like as a check-list of things people should be
trying to hit with whatever details make most sense for the given case.


Thanks,
Roland
  

On 05/01/2014 02:00 PM, Roland McGrath wrote:
>> 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.
> 
> That's an entirely reasonable position.
> It highlights how poor an API it is.
> 
>> On top of that maintainers will likely just do:
> [...]
>> Precisely because positional parameters are harder to read.
>>
>> Then they will typo the params into:
> [...]
> 
> Probably true.  It wasn't a particularly satisfying pattern to begin with.
> 
>> 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.
> 
> I don't object to that.
> 
>> 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.
> 
> I don't see how that is actually typo-prone at all.  A typo anywhere in
> the #define line results in a -Wundef error (aside from writing
> DEFAULT_BAR when that exists but DEFAULT_FOO is what you meant, which
> does not seem so likely).  
> 
> Your following text makes me think you actually meant to write
> "typo-proof" there.  That was pretty meta, dude.

I did mean "typo-proof."

>> 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.
> 
> This works, but it seems rather cumbersome.  I'd say "best practice" is
> to avoid macro APIs altogether.  When doing so would violate some other
> best practice such as avoiding code duplication, then one should craft a
> scheme that optimizes for the characteristics we like as best one can in
> the particular case.  For cases that have fewer parameters than the
> iconv/skeleton.c case, some more concise scheme might have all the
> desireable characteristics.  It seems most important to document those
> characteristics we like as a check-list of things people should be
> trying to hit with whatever details make most sense for the given case.

I agree.

OK, first draft done, and sent to the list.

Cheers,
Carlos.