Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


On Fri, Jan 14, 2011 at 7:44 PM, Lionel Elie Mamane <lionel@mamane.lu> wrote:
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.

According to the 1999 ISO C standard (C99), size_t is an _unsigned_
integer type of at least 16 bits.
It's going to be very hard to write portable code if we assume that
the target do not follow the standard and can't be beaten in
compliance with some header hackery...


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

That's a bug. you should use int8_t or uint8_t for that purpose (or
some equivalent non-ambiguous typedef), never char
or you are going to have bugs. for the very reason you mentioned that
the signed nature of char is unspecified.


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.

That is not too much of a problem. ssize_t can be defined for those
platform if any.

I think the morale of this argument is:
If you want to write portable code, don't rely on non-portable construct.
otherwise you're going to have warning at best, bug at worse on some platforms.

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

Or: don't use AssertPtrSize(a,b) when it is not appropriate:
for instance
the
 AssertPtrSize(pv, csn * isizeof(short));
can be coded
 AssertPtr(pv) && assert(csn >= 0)
or
 AssertPtrSize(pv, csn)  (which achieve the same intended result - the
original assert was clearly not there to detect overflow)

then of course there is the hammer solution:
#define AssertPtrSize(pv, cb) Assert(((ssize_t)(cb)) >= 0 &&
ValidReadPtrSize((pv), cb))

you hide everything under the carpet all the same, but at least you
change only one line, not all the call-site.

Norbert


--
Lionel
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice


Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.