Fix ONE_DIRECTION undef warnings.

Here is another attempt to fix some undef warnings.  The only use of
ONE_DIRECTION is in iconv/skeleton.c and the only setting of it is in
iconv/gconv_simple.c (where it is set to 1).  This patch checks (in
skeleton.c) to see if it is set and then sets it to 0 if it is not.
The use of it, later in skeleton.c is already done with a '#if' so
nothing needed to be changed there.

Tested on mips-mti-linux-gnu, the only code change was to the line
number info in the assert calls.

Steve Ellcey
sellcey@mips.com


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

	* iconf/skelenton.c (ONE_DIRECTION): Set default value if not set.

OK

On 04/28/2014 01:41 PM, Roland McGrath wrote:
> OK

Would it have been better to clarify the interface and have all
the converters define ONE_DIRECTION as either 0 or 1, and that way
we have *explicit* definition of intent from all the converters
including iconv/skeleton.c? With no `ifndef' in skeleton.c which
has most of the same problems as we had before?

Cheers,
Carlos.

On Tue, 2014-04-29 at 15:33 -0400, Carlos O'Donell wrote:
> On 04/28/2014 01:41 PM, Roland McGrath wrote:
> > OK
> 
> Would it have been better to clarify the interface and have all
> the converters define ONE_DIRECTION as either 0 or 1, and that way
> we have *explicit* definition of intent from all the converters
> including iconv/skeleton.c? With no `ifndef' in skeleton.c which
> has most of the same problems as we had before?
> 
> Cheers,
> Carlos.

I think my main issue with this is how many new (duplicate) definitions
it adds.  This is even more of an issue with something like
HP_SMALL_TIMING_AVAIL.  Only one platform (alpha) ever sets this to 1.
But with the current setup, to ensure it always has a value, we have to
define it to 0 in 9 different hp-timing.h headers.  That replication
bothers me and I would like to have one default value defined somewhere
that could be included by the platform specific hp-timing.h files
instead of defining it in all 9 of the non-alpha hp-timing.h header
files.  But there doesn't seem to be an existing infrastructure for
that, and I am not sure if a patch to create such a setup would be
accepted and I don't know if it should be designed just for hp-timing.h
or if it should be a more global header file that other platform headers
could also include to provide default values for other macros.

Steve Ellcey
sellcey@mips.com

On Tue, 29 Apr 2014, Steve Ellcey wrote:

> I think my main issue with this is how many new (duplicate) definitions
> it adds.  This is even more of an issue with something like
> HP_SMALL_TIMING_AVAIL.  Only one platform (alpha) ever sets this to 1.
> But with the current setup, to ensure it always has a value, we have to
> define it to 0 in 9 different hp-timing.h headers.  That replication
> bothers me and I would like to have one default value defined somewhere
> that could be included by the platform specific hp-timing.h files
> instead of defining it in all 9 of the non-alpha hp-timing.h header
> files.  But there doesn't seem to be an existing infrastructure for
> that, and I am not sure if a patch to create such a setup would be
> accepted and I don't know if it should be designed just for hp-timing.h
> or if it should be a more global header file that other platform headers
> could also include to provide default values for other macros.

I don't see a problem with using #include_next in the hp-timing.h case, to 
include generic defaults then override them selectively.

On 04/29/2014 04:11 PM, Steve Ellcey wrote:
> On Tue, 2014-04-29 at 15:33 -0400, Carlos O'Donell wrote:
>> On 04/28/2014 01:41 PM, Roland McGrath wrote:
>>> OK
>>
>> Would it have been better to clarify the interface and have all
>> the converters define ONE_DIRECTION as either 0 or 1, and that way
>> we have *explicit* definition of intent from all the converters
>> including iconv/skeleton.c? With no `ifndef' in skeleton.c which
>> has most of the same problems as we had before?
>>
>> Cheers,
>> Carlos.
> 
> I think my main issue with this is how many new (duplicate) definitions
> it adds.  This is even more of an issue with something like
> HP_SMALL_TIMING_AVAIL.  Only one platform (alpha) ever sets this to 1.
> But with the current setup, to ensure it always has a value, we have to
> define it to 0 in 9 different hp-timing.h headers.  That replication
> bothers me and I would like to have one default value defined somewhere
> that could be included by the platform specific hp-timing.h files
> instead of defining it in all 9 of the non-alpha hp-timing.h header
> files.  But there doesn't seem to be an existing infrastructure for
> that, and I am not sure if a patch to create such a setup would be
> accepted and I don't know if it should be designed just for hp-timing.h
> or if it should be a more global header file that other platform headers
> could also include to provide default values for other macros.

That is a *very* good question and you raise a valid point.

I'm not against a master "default" hp-timing.h that does:

/* This is the default... */
#define HP_SMALL_TIMING_AVAIL 0

I'm also not bothered by having each hp-timing.h define it either.

The HP_SMALL_TIMING_AVAIL is a statement of interface that each of 
the targets should be making.

By using ifdef we run the risk of targets getting unintended semantics.
It's exactly the problem we're trying to avoid. We want the machine
maintainers to make that conscious choice when they setup the low-level
support e.g. "Do I or Don't I support X, Y, and Z?"

That conscious decision can be either:

#define HP_SMALL_TIMING_AVAIL 0

or

#include <hp-timing-defaults.h>

Part of the problem with fixing these issues is setting up a good
"best practice" for this kind of fix.

I am all for having a common hp-timing.h that you can include with
#include_next <...>.

Then Alpha does:
/* Our foo is different.  */
#undef FOO
#define FOO

I was looking at a similar pattern for unifying Linux UAPI constants.

I think that a single mega-header for all these kinds of
constants would be overkill. We might refactor up to one header
per sub-system.

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

What we want is:

#define ONE_DIRECTION 0

Either compiles and you get the semantics you want, or it fails to
compile with an error. Similarly if all uses are #if, then you
get the -Wundef error for using something you didn't define.

Does that answer your question?

Cheers,
Carlos.