[cfe-commits] r146482 - /cfe/trunk/lib/AST/ASTContext.cpp

[Marshall Clow]

On Dec 13, 2011, at 10:34 AM, Chris Lattner wrote:

> Honest question: why would that be better?

Because using
	(uint64_t)(-1)
makes two assumptions, both of which are almost always true:
	* That an "all ones" representation is the max value for uint64_t
and * That the conversion from (int) -1 ==> uint64_t produces the bit patten that you think you want.

What the code wants here is not really an "all ones" pattern, but rather the max value.
And the standard library has a defined, portable way of getting the max value.

On most machines, it will be 0xFFFFFFFFFFFFFFFF (i.e, all ones) and the code that I suggest replacing will work.

The standard library call encapsulates any possible weirdnesses in the construction/conversion process.

-- Marshall

> 
> -Chris
> 
> On Dec 13, 2011, at 9:52 AM, Marshall Clow <mclow.lists at gmail.com> wrote:
> 
>> On Dec 13, 2011, at 9:37 AM, Peter Cooper wrote:
>> 
>>> Hi Abramo
>>> 
>>> We're getting some buildbot failures on the gcc test suite since this change.  The test has a very large union in it which must be triggering your new assert.
>>> 
>>> Can you please have a look and see if your assert is correct on this test?
>>> 
>>> The test in question is gcc.c-torture/execute/991014-1.c
>> 
>> A coding style suggestion (while you're at it).
>> Instead of
>>       (uint64_t)(-1)
>> you should use:
>>       std::numeric_limits<uint64_t>::max ();
>> (and include <limits> if necessary)
>> 
>>>> +    assert((Size == 0 || EltInfo.first <= (uint64_t)(-1)/Size) && "Overflow in array type bit size evaluation");
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits