Re: [Libreoffice] debug build fails because graphite ./configure not up to date

On Sat, Jan 15, 2011 at 01:38:34AM +0100, Christian Lohmaier wrote:
> On Sat, Jan 15, 2011 at 12:43 AM, Lionel Elie Mamane <lionel@mamane.lu> wrote:

> > The problem is that graphite does not build with -Werror in debug
> > mode; one of the least fixable reasons is that it uses asserts left
> > and right, asserts are *designed* to always be true, sometimes gcc
> > detects that they are (e.g. when comparing an unsigned integer type
> > with 0), this raises a warning that gets treated as error because
> > of -Werror.

> Sorry - but when checking whether an unsigned type is larger or
> equals zero, then this is a bug in the assertion and the compiler
> rightfully complains about stupid code that could be written as
> "true" instead.

Well, not necessarily. The type is unsigned on the OS/architecture
being currently compiled for, but not necessarily on all
OS/architectures. E.g. size_t was signed on some older unices. Or
"char", which is allowed by the C and C++ standards to be either
signed or unsigned. E.g. in case you use "char" as a "tiny int" for
values between 0 and 10, you'd assert "value >=0 && value <= 10".

Or also, when defining a preprocessor macro: the macro can be used
with _any_ type, signed or unsigned. When the type is unsigned (part
of) what it does is useless, but hey, the compiler will optimise it
away. And the test makes sense were the macro is used with a signed
type. =

> But maybe I misunderstand... Do you have an example?

graphite file engine/include/graphite/GrDebug.h:

 #define AssertPtrSize(pv, cb) Assert((cb) >= 0 && ValidReadPtrSize((pv), cb))

graphite file: engine/src/generic/Util.cpp:

Macro used the first time with a signed type in second argument:

void SwapBytes(void * pv1, void * pv2, int cb)
{
        AssertPtrSize(pv1, cb);
        AssertPtrSize(pv2, cb);

And now with an unsigned type:

void FillShorts(void * pv, short sn, int csn)
{
        AssertPtrSize(pv, csn * isizeof(short));

It makes total sense that the macro compares to zero, else it would
not catch some assert-able error in the first case!

Now that I look more closely at it, I see (in GrCommon.h) the authors intended
to address that issue:

// This is to make a signed isizeof operator, otherwise we get tons of warnings about
// signed / unsigned mismatches.
#define isizeof(T) (sizeof(T))

Well, that ain't gonna work, it is not making any cast... Maybe
something like

#define isizeof(T) ((ssize_t) sizeof(T))

would work (but maybe loose some portability to
OS/architectures/compilers that don't define ssize_t. Or possibly

#define isizeof(T) ((ptrdiff_t) sizeof(T))

would be available "everywhere" and *maybe* safe (as in ptrdiff_t will
always be big enough).

-- 
Lionel