This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] Fix ONE_DIRECTION undef warnings.


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]